Use after free in ami(4)

[Arto Jonsson]

Spotted by LLVM static analyser.

In /usr/src/sys/dev/ic/ami.c line 499 ami_freemem(sc, am) is called. In
the next block ami_alloc_ccbs(...) is called. If the result != 0 control
jumps to free_mbox in line 614. This falls through to call
ami_freemem(sc, am) again.

[Kenneth R Westerback]

Is LLMV happy with this diff?

.... Ken

Index: ami.c
===================================================================
RCS file: /cvs/src/sys/dev/ic/ami.c,v
retrieving revision 1.223
diff -u -p -r1.223 ami.c
--- ami.c 9 Jan 2012 18:50:44 -0000 1.223
+++ ami.c 4 May 2013 23:02:16 -0000
@@ -497,6 +497,7 @@ ami_attach(struct ami_softc *sc)
}

ami_freemem(sc, am);
+ am = NULL;

if (ami_alloc_ccbs(sc, AMI_MAXCMDS + 1) != 0) {
/* error already printed */
@@ -614,7 +615,8 @@ ami_attach(struct ami_softc *sc)
free_mbox:
ami_freemem(sc, sc->sc_mbox_am);
free_idata:
- ami_freemem(sc, am);
+ if (am)
+ ami_freemem(sc, am);

return (1);
}

[Jonathan Gray]

here is the diff I had for ami, the mfi one included here as a bonus

Index: ami.c
===================================================================
RCS file: /cvs/src/sys/dev/ic/ami.c,v
retrieving revision 1.223
diff -u -p -r1.223 ami.c
--- ami.c 9 Jan 2012 18:50:44 -0000 1.223

+++ ami.c 5 May 2013 01:06:08 -0000
@@ -496,12 +496,12 @@ ami_attach(struct ami_softc *sc)
sc->sc_link.openings = sc->sc_maxcmds;
}

- ami_freemem(sc, am);
-

if (ami_alloc_ccbs(sc, AMI_MAXCMDS + 1) != 0) {
/* error already printed */

goto free_mbox;
}
+
+ ami_freemem(sc, am);

/* hack for hp netraid version encoding */
if ('A' <= sc->sc_fwver[2] && sc->sc_fwver[2] <= 'Z' &&
Index: mfi.c
===================================================================
RCS file: /cvs/src/sys/dev/ic/mfi.c,v
retrieving revision 1.144
diff -u -p -r1.144 mfi.c
--- mfi.c 3 May 2013 02:46:28 -0000 1.144
+++ mfi.c 3 May 2013 07:20:03 -0000
@@ -1470,8 +1470,10 @@ mfi_bio_getitall(struct mfi_softc *sc)
if (cfg == NULL)
goto done;
if (mfi_mgmt(sc, MR_DCMD_CONF_GET, MFI_DATA_IN, sizeof *cfg, cfg,
- NULL))
+ NULL)) {
+ free(cfg, M_DEVBUF);
goto done;
+ }

size = cfg->mfc_size;
free(cfg, M_DEVBUF);

[Kenneth R Westerback]

Looks better to me. ok krw@ for both.

.... Ken

>
> Index: ami.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/ic/ami.c,v
> retrieving revision 1.223
> diff -u -p -r1.223 ami.c
> --- ami.c 9 Jan 2012 18:50:44 -0000 1.223

[Federico Schwindt]

ok to both.

there are more cases in mfi_bio_getitall() where the second allocated cfg
(ld_det as well) might not be freed if something goes wrong.

f.-