32 Memory Leaks on Error Paths

Hi All,

I'm with the Stanford Meta-level Compilation research group, and I have a set of memory leaks on error paths for the 2.6.0-test5 kernel. (I also have error reports for 2.4.18 and a couple other kernels if anyone is interested).

There may be one or more "GOTO -->" markers showing the different paths of execution that can occur between where the memory is allocated and where the function returns.

My checker identifies error paths with a learning algorithm on features surrounding goto and return statements. I'd greatly appreciate any comments or confirmation on these bugs.

Thanks!

---
David Yu Chen
http://www.stanford.edu/~dychen/

---------------------------------------------------------

Count = 32

# | File
1 | drivers/char/vt_ioctl.c
1 | drivers/media/video/bttv-risc.c
2 | drivers/mtd/chips/cfi_cmdset_0020.c
1 | drivers/mtd/mtdblock.c
1 | drivers/net/irda/ali-ircc.c
1 | drivers/net/irda/nsc-ircc.c
2 | drivers/pci/hotplug/ibmphp_ebda.c
1 | drivers/scsi/NCR_Q720.c
1 | drivers/scsi/scsi_debug.c
1 | drivers/telephony/ixj_pcmcia.c
2 | drivers/usb/class/usb-midi.c
1 | drivers/usb/input/hiddev.c
1 | fs/afs/cell.c
2 | fs/jffs2/scan.c
1 | fs/nfsd/nfsctl.c
2 | net/ipv4/igmp.c
1 | net/irda/irlmp.c
1 | net/sunrpc/auth_gss/gss_mech_switch.c
1 | net/sunrpc/auth_gss/gss_pseudoflavors.c
1 | security/selinux/selinuxfs.c
1 | security/selinux/ss/mls.c
4 | security/selinux/ss/policydb.c
1 | sound/oss/emu10k1/midi.c
1 | sound/oss/ymfpci.c

---------------------------------------------------------

[FILE: 2.6.0-test5/drivers/char/vt_ioctl.c]
[FUNC: do_kdsk_ioctl]
[LINES: 133-150]
[VAR: key_map]
128:
129: if (keymap_count >= MAX_NR_OF_USER_KEYMAPS &&
130: !capable(CAP_SYS_RESOURCE))
131: return -EPERM;
132:
START -->
133: key_map = (ushort *) kmalloc(sizeof(plain_map),
134: GFP_KERNEL);
135: if (!key_map)
136: return -ENOMEM;
137: key_maps[s] = key_map;

 138:			key_map[0] = U(K_ALLOCATED);
        ... DELETED 6 lines ...

145: break; /* nothing to do */
146: /*
147: * Attention Key.
148: */
149: if (((ov == K_SAK) || (v == K_SAK)) && !capable(CAP_SYS_ADMIN))
END -->
150: return -EPERM;
151: key_map[i] = U(v);
152: if (!s && (KTYP(ov) == KT_SHIFT || KTYP(v) == KT_SHIFT))
153: compute_shiftstate();
154: break;
155: }
---------------------------------------------------------

[FILE: 2.6.0-test5/drivers/media/video/bttv-risc.c] [FUNC: bttv_risc_overlay]
[LINES: 214-223]
[VAR: skips]
209: struct btcx_skiplist *skips;
210: u32 *rp,ri,ra;
211: u32 addr;
212:
213: /* skip list for window clipping */
START -->
214: if (NULL == (skips = kmalloc(sizeof(*skips) * ov->nclips,GFP_KERNEL)))
215: return -ENOMEM;
216:
217: /* estimate risc mem: worst case is (clip+1) * lines instructions
218: + sync + jump (all 2 dwords) */
219: instructions = (ov->nclips + 1) *
220: ((skip_even || skip_odd) ? ov->w.height>>1 : ov->w.height);
221: instructions += 2;
222: if ((rc = btcx_riscmem_alloc(btv->dev,risc,instructions*8)) < 0)
END -->
223: return rc;
224:
225: /* sync instruction */
226: rp = risc->cpu;
227: *(rp++) = cpu_to_le32(BT848_RISC_SYNC|BT848_FIFO_STATUS_FM1);
228: *(rp++) = cpu_to_le32(0);
---------------------------------------------------------

[FILE: 2.6.0-test5/drivers/mtd/chips/cfi_cmdset_0020.c] [FUNC: cfi_staa_setup]
[LINES: 191-211]
[VAR: mtd]
186: struct mtd_info *mtd;
187: unsigned long offset = 0;
188: int i,j;
189: unsigned long devsize = (1<<cfi->cfiq->DevSize) * cfi->interleave;
190:
START -->
191: mtd = kmalloc(sizeof(*mtd), GFP_KERNEL);
192: //printk(KERN_DEBUG "number of CFI chips: %d\n", cfi->numchips);
193:
194: if (!mtd) {
195: printk(KERN_ERR "Failed to allocate memory for MTD device\n");

 196:		kfree(cfi->cmdset_priv);
        ... DELETED 9 lines ...

206: mtd->eraseregions = kmalloc(sizeof(struct mtd_erase_region_info)
207: * mtd->numeraseregions, GFP_KERNEL);
208: if (!mtd->eraseregions) {
209: printk(KERN_ERR "Failed to allocate memory for MTD erase region info\n");
210: kfree(cfi->cmdset_priv);
END -->
211: return NULL;
212: }
213:
214: for (i=0; i<cfi->cfiq->NumEraseRegions; i++) {
215: unsigned long ernum, ersize;
216: ersize = ((cfi->cfiq->EraseRegionInfo[i] >> 8) & ~0xff) * cfi->interleave;
---------------------------------------------------------

[FILE: 2.6.0-test5/drivers/mtd/chips/cfi_cmdset_0020.c] [FUNC: cfi_staa_setup]
[LINES: 191-235]
[VAR: mtd]
186: struct mtd_info *mtd;
187: unsigned long offset = 0;
188: int i,j;
189: unsigned long devsize = (1<<cfi->cfiq->DevSize) * cfi->interleave;
190:
START -->
191: mtd = kmalloc(sizeof(*mtd), GFP_KERNEL);
192: //printk(KERN_DEBUG "number of CFI chips: %d\n", cfi->numchips);
193:
194: if (!mtd) {
195: printk(KERN_ERR "Failed to allocate memory for MTD device\n");

 196:		kfree(cfi->cmdset_priv);
        ... DELETED 33 lines ...

230: if (offset != devsize) {
231: /* Argh */
232: printk(KERN_WARNING "Sum of regions (%lx) != total size of set of interleaved chips (%lx)\n", offset, devsize);
233: kfree(mtd->eraseregions);
234: kfree(cfi->cmdset_priv);
END -->
235: return NULL;
236: }
237:
238: for (i=0; i<mtd->numeraseregions;i++){
239: printk(KERN_DEBUG "%d: offset=0x%x,size=0x%x,blocks=%d\n",
240: i,mtd->eraseregions[i].offset,
---------------------------------------------------------

looks like checking for mtdblks instead of mtdblk
[FILE: 2.6.0-test5/drivers/mtd/mtdblock.c]
[FUNC: mtdblock_open]
[LINES: 277-279]
[VAR: mtdblk]
272: mtdblks[dev]->count++;
273: return 0;
274: }
275:
276: /* OK, it's not open. Create cache info for it */
START -->
277: mtdblk = kmalloc(sizeof(struct mtdblk_dev), GFP_KERNEL);
278: if (!mtdblks)
END -->
279: return -ENOMEM;
280:
281: memset(mtdblk, 0, sizeof(*mtdblk));
282: mtdblk->count = 1;
283: mtdblk->mtd = mtd;
284:
---------------------------------------------------------

[FILE: 2.6.0-test5/drivers/net/irda/ali-ircc.c]
[FUNC: ali_ircc_open]
[LINES: 259-337]
[VAR: self]
254: /* Set FIR FIFO and DMA Threshold */
255: if ((ali_ircc_setup(info)) == -1)
256: return -1;
257:
258: /* Allocate new instance of the driver */
START -->
259: self = kmalloc(sizeof(struct ali_ircc_cb), GFP_KERNEL);
260: if (self == NULL)
261: {
262: ERROR("%s(), can't allocate memory for control block!\n", __FUNCTION__);
263: return -ENOMEM;
264: }

        ... DELETED 67 lines ...

332: self->tx_fifo.len = self->tx_fifo.ptr = self->tx_fifo.free = 0;
333: self->tx_fifo.tail = self->tx_buff.head;
334:
335: if (!(dev = dev_alloc("irda%d", &err))) {
336: ERROR("%s(), dev_alloc() failed!\n", __FUNCTION__);
END -->
337: return -ENOMEM;
338: }
339:
340: dev->priv = (void *) self;
341: self->netdev = dev;
342:
---------------------------------------------------------

[FILE: 2.6.0-test5/drivers/net/irda/nsc-ircc.c]
[FUNC: nsc_ircc_open]
[LINES: 265-341]
[VAR: self]
260: return -1;
261:
262: MESSAGE("%s, driver loaded (Dag Brattli)\n", driver_name);
263:
264: /* Allocate new instance of the driver */
START -->
265: self = kmalloc(sizeof(struct nsc_ircc_cb), GFP_KERNEL);
266: if (self == NULL) {
267: ERROR("%s(), can't allocate memory for "
268: "control block!\n", __FUNCTION__);
269: return -ENOMEM;
270: }

        ... DELETED 65 lines ...

336: self->tx_fifo.len = self->tx_fifo.ptr = self->tx_fifo.free = 0;
337: self->tx_fifo.tail = self->tx_buff.head;
338:
339: if (!(dev = dev_alloc("irda%d", &err))) {
340: ERROR("%s(), dev_alloc() failed!\n", __FUNCTION__);
END -->
341: return -ENOMEM;
342: }
343:
344: dev->priv = (void *) self;
345: self->netdev = dev;
346:
---------------------------------------------------------

[FILE: 2.6.0-test5/drivers/pci/hotplug/ibmphp_ebda.c] [FUNC: ebda_rsrc_controller]
[LINES: 857-1064]
[VAR: bus_info_ptr1]
852:
853: // create bus_info lined list --- if only one slot per bus: slot_min = slot_max
854:
855: bus_info_ptr2 = ibmphp_find_same_bus_num (slot_ptr->slot_bus_num);
856: if (!bus_info_ptr2) {
START -->
857: bus_info_ptr1 = (struct bus_info *) kmalloc (sizeof (struct bus_info), GFP_KERNEL);
858: if (!bus_info_ptr1) {
859: rc = -ENOMEM;
860: goto error_no_hp_slot;
861: }

 862:				memset (bus_info_ptr1, 0, sizeof (struct bus_info));
        ... DELETED 79 lines ...

942: hpc_ptr->irq = readb (io_mem + addr + 5);
943: addr += 6;
944: break;
945: default:
946: rc = -ENODEV;
GOTO -->
947: goto error_no_hp_slot;
948: }
949:
950: //reorganize chassis' linked list
951: combine_wpg_for_chassis ();

 952:		combine_wpg_for_expansion ();
        ... DELETED 106 lines ...

1059: kfree (hp_slot_ptr);
1060:error_no_hp_slot:
1061: free_ebda_hpc (hpc_ptr);
1062:error_no_hpc:
1063: iounmap (io_mem);
END -->
1064: return rc;
1065:}
1066:
1067:/*
1068: * map info (bus, devfun, start addr, end addr..) of i/o, memory, 1069: * pfm from the physical addr to a list of resource.
---------------------------------------------------------

[FILE: 2.6.0-test5/drivers/pci/hotplug/ibmphp_ebda.c] [FUNC: ebda_rsrc_controller]
[LINES: 981-1064]
[VAR: tmp_slot]
976: if (!hp_slot_ptr->name) {
977: rc = -ENOMEM;
978: goto error_no_hp_name;
979: }
980:
START -->
981: tmp_slot = kmalloc(sizeof(*tmp_slot), GFP_KERNEL);
982: if (!tmp_slot) {
983: rc = -ENOMEM;
984: goto error_no_slot;
985: }

 986:			memset(tmp_slot, 0, sizeof(*tmp_slot));
        ... DELETED 17 lines ...

1004: tmp_slot->bus = hpc_ptr->slots[index].slot_bus_num;
1005:
1006: bus_info_ptr1 = ibmphp_find_same_bus_num (hpc_ptr->slots[index].slot_bus_num);
1007: if (!bus_info_ptr1) {
1008: rc = -ENODEV;
GOTO -->
1009: goto error;
1010: }
1011: tmp_slot->bus_on = bus_info_ptr1;
1012: bus_info_ptr1 = NULL;
1013: tmp_slot->ctrl = hpc_ptr;
1014:

        ... DELETED 44 lines ...

1059: kfree (hp_slot_ptr);
1060:error_no_hp_slot:
1061: free_ebda_hpc (hpc_ptr);
1062:error_no_hpc:
1063: iounmap (io_mem);
END -->
1064: return rc;
1065:}
1066:
1067:/*
1068: * map info (bus, devfun, start addr, end addr..) of i/o, memory, 1069: * pfm from the physical addr to a list of resource.
---------------------------------------------------------

[FILE: 2.6.0-test5/drivers/scsi/NCR_Q720.c]
[FUNC: NCR_Q720_probe]
[LINES: 153-182]
[VAR: p]
148: __u8 pos2, pos4, asr2, asr9, asr10;
149: __u16 io_base;
150: __u32 base_addr, mem_size;
151: __u32 mem_base;
152:
START -->
153: p = kmalloc(sizeof(*p), GFP_KERNEL);
154: if (!p)
155: return -ENOMEM;
156:
157: memset(p, 0, sizeof(*p));

 158:	pos2 = mca_device_read_pos(mca_dev, 2);
        ... DELETED 18 lines ...

177:
178: /* sanity check I/O mapping */
179: i = inb(io_base) | (inb(io_base+1)<<8);
180: if(i != NCR_Q720_MCA_ID) {
181: printk(KERN_ERR "NCR_Q720, adapter failed to I/O map registers correctly at 0x%x(0x%x)\n", io_base, i);
END -->
182: return -ENODEV;
183: }
184:
185: /* Phase II, find the ram base and memory map the board register */
186: pos4 = inb(io_base + 4);
187: /* enable streaming data */
---------------------------------------------------------

Leaks memory if kmalloc fails between 0 .. devs_per_host [FILE: 2.6.0-test5/drivers/scsi/scsi_debug.c] [FUNC: sdebug_add_adapter]
[LINES: 1612-1652]
[VAR: sdbg_devinfo]
1607: memset(sdbg_host, 0, sizeof(*sdbg_host));
1608: INIT_LIST_HEAD(&sdbg_host->dev_info_list);
1609:
1610: devs_per_host = scsi_debug_num_tgts * scsi_debug_max_luns;
1611: for (k = 0; k < devs_per_host; k++) {
START -->
1612: sdbg_devinfo = kmalloc(sizeof(*sdbg_devinfo),GFP_KERNEL);
1613: if (NULL == sdbg_devinfo) {
1614: printk(KERN_ERR "%s: out of memory at line %d\n",
1615: __FUNCTION__, __LINE__);
1616: error = -ENOMEM;
GOTO -->

1617:			goto clean1;
        ... DELETED 29 lines ...

1647: kfree(sdbg_devinfo);
1648: }
1649:
1650:clean1:
1651: kfree(sdbg_host);
END -->
1652: return error;
1653:}
1654:
1655:static void sdebug_remove_adapter()
1656:{
1657: struct sdebug_host_info * sdbg_host = NULL;
---------------------------------------------------------

[FILE: 2.6.0-test5/drivers/telephony/ixj_pcmcia.c] [FUNC: ixj_attach]
[LINES: 53-64]
[VAR: link]
48: client_reg_t client_reg;
49: dev_link_t *link;
50: int ret;
51: DEBUG(0, "ixj_attach()\n");
52: /* Create new ixj device */
START -->
53: link = kmalloc(sizeof(struct dev_link_t), GFP_KERNEL);
54: if (!link)
55: return NULL;
56: memset(link, 0, sizeof(struct dev_link_t));
57: link->io.Attributes1 = IO_DATA_PATH_WIDTH_8;
58: link->io.Attributes2 = IO_DATA_PATH_WIDTH_8;
59: link->io.IOAddrLines = 3;
60: link->conf.Vcc = 50;
61: link->conf.IntType = INT_MEMORY_AND_IO;
62: link->priv = kmalloc(sizeof(struct ixj_info_t), GFP_KERNEL);
63: if (!link->priv)
END -->
64: return NULL;
65: memset(link->priv, 0, sizeof(struct ixj_info_t));
66: /* Register with Card Services */
67: link->next = dev_list;
68: dev_list = link;
69: client_reg.dev_info = &dev_info;
---------------------------------------------------------

Leaks if devices == 0 ? Error_end only frees mdevs if (devices > 0), but for mdevs=kmalloc(0), the slab allocator may still actually return memory [FILE: 2.6.0-test5/drivers/usb/class/usb-midi.c]
[FUNC: alloc_usb_midi_device]
[LINES: 1621-1772]
[VAR: mdevs]
1616: devices = inDevs > outDevs ? inDevs : outDevs;
1617: devices = maxdevices > devices ? devices : maxdevices;
1618:
1619: /* obtain space for device name (iProduct) if not known. */
1620: if ( ! u->deviceName ) {
START -->
1621: mdevs = (struct usb_mididev **)
1622: kmalloc(sizeof(struct usb_mididevs *)*devices
1623: + sizeof(char) * 256, GFP_KERNEL);
1624: } else {
1625: mdevs = (struct usb_mididev **)

1626:			kmalloc(sizeof(struct usb_mididevs *)*devices, GFP_KERNEL);
        ... DELETED 83 lines ...

1710: }
1711: mout = mouts[outEndpoint];
1712:
1713: mdevs[i] = allocMidiDev( s, min, mout, inCableId, outCableId );
1714: if ( mdevs[i] == NULL )
GOTO -->
1715: goto error_end;
1716:
1717: }
1718:
1719: /* Success! */

1720:	for ( i=0 ; i<devices ; i++ ) {
        ... DELETED 46 lines ...

1767: if ( mouts[i] != NULL ) {
1768: remove_midi_out_endpoint( mouts[i] );
1769: }
1770: }
1771:
END -->
1772: return -ENOMEM;
1773:}
1774:
1775:/* ------------------------------------------------------------------------- */ 1776:
1777:/** Attempt to scan YAMAHA's device descriptor and detect correct values of
---------------------------------------------------------

[FILE: 2.6.0-test5/drivers/usb/class/usb-midi.c]
[FUNC: alloc_usb_midi_device]
[LINES: 1625-1772]
[VAR: mdevs]
1620: if ( ! u->deviceName ) {
1621: mdevs = (struct usb_mididev **)
1622: kmalloc(sizeof(struct usb_mididevs *)*devices
1623: + sizeof(char) * 256, GFP_KERNEL);
1624: } else {
START -->
1625: mdevs = (struct usb_mididev **)
1626: kmalloc(sizeof(struct usb_mididevs *)*devices, GFP_KERNEL);
1627: }
1628:
1629: if ( !mdevs ) {
1630: /* devices = 0; */

        ... DELETED 79 lines ...

1710: }
1711: mout = mouts[outEndpoint];
1712:
1713: mdevs[i] = allocMidiDev( s, min, mout, inCableId, outCableId );
1714: if ( mdevs[i] == NULL )
GOTO -->
1715: goto error_end;
1716:
1717: }
1718:
1719: /* Success! */

1720:	for ( i=0 ; i<devices ; i++ ) {
        ... DELETED 46 lines ...

1767: if ( mouts[i] != NULL ) {
1768: remove_midi_out_endpoint( mouts[i] );
1769: }
1770: }
1771:
END -->
1772: return -ENOMEM;
1773:}
1774:
1775:/* ------------------------------------------------------------------------- */ 1776:
1777:/** Attempt to scan YAMAHA's device descriptor and detect correct values of
---------------------------------------------------------

[FILE: 2.6.0-test5/drivers/usb/input/hiddev.c]
[FUNC: hiddev_connect]
[LINES: 723-730]
[VAR: hiddev]
718: break;
719:
720: if (i == hid->maxcollection && (hid->quirks & HID_QUIRK_HIDDEV) == 0)
721: return -1;
722:
START -->
723: if (!(hiddev = kmalloc(sizeof(struct hiddev), GFP_KERNEL)))
724: return -1;
725: memset(hiddev, 0, sizeof(struct hiddev));
726:
727: retval = usb_register_dev(&hiddev->intf, &hiddev_class);
728: if (retval) {
729: err("Not able to get a minor for this device.");
END -->
730: return -1;
731: }
732:
733: init_waitqueue_head(&hiddev->wait);
734:
735: hiddev->minor = hiddev->intf.minor;
---------------------------------------------------------

[FILE: 2.6.0-test5/fs/afs/cell.c]
[FUNC: afs_cell_create]
[LINES: 58-129]
[VAR: cell]
53: if (!name) BUG(); /* TODO: want to look up "this cell" in the cache */
54:
55: down_write(&afs_cells_sem);
56:
57: /* allocate and initialise a cell record */
START -->
58: cell = kmalloc(sizeof(afs_cell_t) + strlen(name) + 1,GFP_KERNEL);
59: if (!cell) {
60: _leave(" = -ENOMEM");
61: return -ENOMEM;
62: }
63:

        ... DELETED 25 lines ...

89:
90: if (sscanf(vllist,"%u.%u.%u.%u",&a,&b,&c,&d)!=4)
91: goto badaddr;
92:
93: if (a>255 || b>255 || c>255 || d>255)
GOTO -->
94: goto badaddr;
95:
96: cell->vl_addrs[cell->vl_naddrs++].s_addr =
97: htonl((a<<24)|(b<<16)|(c<<8)|d);
98:
99: if (cell->vl_naddrs>=16)

        ... DELETED 24 lines ...

124: badaddr:
125: printk("kAFS: bad VL server IP address: '%s'\n",vllist);
126: error:
127: up_write(&afs_cells_sem);
128: kfree(afs_cell_root);
END -->
129: return ret;
130:} /* end afs_cell_create() */
131:
132:/*****************************************************************************/ 133:/*
134: * initialise the cell database from module parameters
---------------------------------------------------------

[FILE: 2.6.0-test5/fs/jffs2/scan.c]
[FUNC: jffs2_scan_medium]
[LINES: 98-109]
[VAR: flashbuf]
93: buf_size = c->sector_size;
94: else
95: buf_size = PAGE_SIZE;
96:
97: D1(printk(KERN_DEBUG "Allocating readbuf of %d bytes\n", buf_size));
START -->
98: flashbuf = kmalloc(buf_size, GFP_KERNEL);
99: if (!flashbuf)
100: return -ENOMEM;
101: }
102:
103: for (i=0; i<c->nr_blocks; i++) {
104: struct jffs2_eraseblock *jeb = &c->blocks[i];
105:
106: ret = jffs2_scan_eraseblock(c, jeb, buf_size?flashbuf:(flashbuf+jeb->offset), buf_size);
107:
108: if (ret < 0)
END -->
109: return ret;
110:
111: ACCT_PARANOIA_CHECK(jeb);
112:
113: /* Now decide which list to put it on */
114: switch(ret) {
---------------------------------------------------------

[FILE: 2.6.0-test5/fs/jffs2/scan.c]
[FUNC: jffs2_scan_medium]
[LINES: 98-233]
[VAR: flashbuf]
93: buf_size = c->sector_size;
94: else
95: buf_size = PAGE_SIZE;
96:
97: D1(printk(KERN_DEBUG "Allocating readbuf of %d bytes\n", buf_size));
START -->
98: flashbuf = kmalloc(buf_size, GFP_KERNEL);
99: if (!flashbuf)
100: return -ENOMEM;
101: }
102:

 103:	for (i=0; i<c->nr_blocks; i++) {
        ... DELETED 124 lines ...

228: }
229: if (c->nr_erasing_blocks) {
230: if ( !c->used_size && ((empty_blocks+bad_blocks)!= c->nr_blocks || bad_blocks == c->nr_blocks) ) {
231: printk(KERN_NOTICE "Cowardly refusing to erase blocks on filesystem with no valid JFFS2 nodes\n");
232: printk(KERN_NOTICE "empty_blocks %d, bad_blocks %d, c->nr_blocks %d\n",empty_blocks,bad_blocks,c->nr_blocks);
END -->
233: return -EIO;
234: }
235: jffs2_erase_pending_trigger(c);
236: }
237: if (buf_size)
238: kfree(flashbuf);
---------------------------------------------------------

[FILE: 2.6.0-test5/fs/nfsd/nfsctl.c]
[FUNC: TA_write]
[LINES: 105-120]
[VAR: ar]
100: if (file->private_data)
101: return -EINVAL; /* only one write allowed per open */
102: if (size > PAGE_SIZE - sizeof(struct argresp))
103: return -EFBIG;
104:
START -->
105: ar = kmalloc(PAGE_SIZE, GFP_KERNEL);
106: if (!ar)
107: return -ENOMEM;
108: ar->size = 0;
109: down(&file->f_dentry->d_inode->i_sem);
110: if (file->private_data)

        ... DELETED 4 lines ...

115: if (rv) {
116: kfree(ar);
117: return rv;
118: }
119: if (copy_from_user(ar->data, buf, size))
END -->
120: return -EFAULT;
121:
122: rv = write_op[ino](file, ar->data, size);
123: if (rv>0) {
124: ar->size = rv;
125: rv = size;
---------------------------------------------------------

[FILE: 2.6.0-test5/net/ipv4/igmp.c]
[FUNC: igmpv3_newpack]
[LINES: 274-284]
[VAR: skb]
269: struct sk_buff *skb;
270: struct rtable *rt;
271: struct iphdr *pip;
272: struct igmpv3_report *pig;
273:
START -->
274: skb = alloc_skb(size + dev->hard_header_len + 15, GFP_ATOMIC);
275: if (skb == NULL)
276: return 0;
277:
278: {
279: struct flowi fl = { .oif = dev->ifindex,

 280:				    .nl_u = { .ip4_u = {
 281:				    .daddr = IGMPV3_ALL_MCR } },
 282:				    .proto = IPPROTO_IGMP };

283: if (ip_route_output_key(&rt, &fl))
END -->
284: return 0;
285: }
286: if (rt->rt_src == 0) {
287: ip_rt_put(rt);
288: return 0;
289: }
---------------------------------------------------------

[FILE: 2.6.0-test5/net/ipv4/igmp.c]
[FUNC: igmpv3_newpack]
[LINES: 274-288]
[VAR: skb]
269: struct sk_buff *skb;
270: struct rtable *rt;
271: struct iphdr *pip;
272: struct igmpv3_report *pig;
273:
START -->
274: skb = alloc_skb(size + dev->hard_header_len + 15, GFP_ATOMIC);
275: if (skb == NULL)
276: return 0;
277:
278: {
279: struct flowi fl = { .oif = dev->ifindex,

        ... DELETED 3 lines ...

283: if (ip_route_output_key(&rt, &fl))
284: return 0;
285: }
286: if (rt->rt_src == 0) {
287: ip_rt_put(rt);
END -->
288: return 0;
289: }
290:
291: skb->dst = &rt->u.dst;
292: skb->dev = dev;
293:
---------------------------------------------------------

[FILE: 2.6.0-test5/net/irda/irlmp.c]
[FUNC: irlmp_open_lsap]
[LINES: 161-183]
[VAR: self]
156: return NULL;
157: } else if (irlmp_slsap_inuse(slsap_sel))
158: return NULL;
159:
160: /* Allocate new instance of a LSAP connection */
START -->
161: self = kmalloc(sizeof(struct lsap_cb), GFP_ATOMIC);
162: if (self == NULL) {
163: ERROR("%s: can't allocate memory", __FUNCTION__);
164: return NULL;
165: }

 166:	memset(self, 0, sizeof(struct lsap_cb));
        ... DELETED 11 lines ...

178: self->dlsap_sel = LSAP_ANY;
179: /* self->connected = FALSE; -> already NULL via memset() */
180:
181: init_timer(&self->watchdog_timer);
182:
END -->
183: ASSERT(notify->instance != NULL, return NULL;);
184: self->notify = *notify;
185:
186: self->lsap_state = LSAP_DISCONNECTED;
187:
188: /* Insert into queue of unconnected LSAPs */
---------------------------------------------------------

[FILE: 2.6.0-test5/net/sunrpc/auth_gss/gss_mech_switch.c]
[FUNC: gss_mech_register]
[LINES: 67-74]
[VAR: gm]
62:int
63:gss_mech_register(struct xdr_netobj * mech_type, struct gss_api_ops * ops) 64:{
65: struct gss_api_mech *gm;
66:
START -->
67: if (!(gm = kmalloc(sizeof(*gm), GFP_KERNEL))) {
68: printk("Failed to allocate memory in gss_mech_register");
69: return -1;
70: }
71: gm->gm_oid.len = mech_type->len;
72: if (!(gm->gm_oid.data = kmalloc(mech_type->len, GFP_KERNEL))) {
73: printk("Failed to allocate memory in gss_mech_register");
END -->
74: return -1;
75: }
76: memcpy(gm->gm_oid.data, mech_type->data, mech_type->len);
77: /* We're counting the reference in the registered_mechs list: */
78: atomic_set(&gm->gm_count, 1);
79: gm->gm_ops = ops;
---------------------------------------------------------

[FILE: 2.6.0-test5/net/sunrpc/auth_gss/gss_pseudoflavors.c] [FUNC: gss_register_triple]
[LINES: 74-97]
[VAR: triple]
69:gss_register_triple(u32 pseudoflavor, struct gss_api_mech *mech,
70: u32 qop, u32 service)
71:{
72: struct sup_sec_triple *triple;
73:
START -->
74: if (!(triple = kmalloc(sizeof(*triple), GFP_KERNEL))) {
75: printk("Alloc failed in gss_register_triple");
76: goto err;
77: }
78: triple->pseudoflavor = pseudoflavor;

  79:	triple->mech = gss_mech_get_by_OID(&mech->gm_oid);
        ... DELETED 1 lines ...

81: triple->service = service;
82:
83: spin_lock(&registered_triples_lock);
84: if (do_lookup_triple_by_pseudoflavor(pseudoflavor)) {
85: printk("Registered pseudoflavor %d again\n", pseudoflavor);
GOTO -->
86: goto err_unlock;
87: }
88: list_add(&triple->triples, &registered_triples);
89: spin_unlock(&registered_triples_lock);
90: dprintk("RPC: registered pseudoflavor %d\n", pseudoflavor);
91:
92: return 0;
93:
94:err_unlock:
95: spin_unlock(&registered_triples_lock);
96:err:
END -->
97: return -1;
98:}
99:
100:int
101:gss_unregister_triple(u32 pseudoflavor)
102:{
---------------------------------------------------------

[FILE: 2.6.0-test5/security/selinux/selinuxfs.c]
[FUNC: TA_write]
[LINES: 253-269]
[VAR: ar]
248: if (file->private_data)
249: return -EINVAL; /* only one write allowed per open */
250: if (size > PAYLOAD_SIZE - 1) /* allow one byte for null terminator */
251: return -EFBIG;
252:
START -->
253: ar = kmalloc(PAGE_SIZE, GFP_KERNEL);
254: if (!ar)
255: return -ENOMEM;
256: memset(ar, 0, PAGE_SIZE); /* clear buffer, particularly last byte */
257: ar->size = 0;

 258:	down(&file->f_dentry->d_inode->i_sem);
        ... DELETED 5 lines ...

264: if (rv) {
265: kfree(ar);
266: return rv;
267: }
268: if (copy_from_user(ar->data, buf, size))
END -->
269: return -EFAULT;
270:
271: rv = write_op[ino](file, ar->data, size);
272: if (rv>0) {
273: ar->size = rv;
274: rv = size;
---------------------------------------------------------

[FILE: 2.6.0-test5/security/selinux/ss/mls.c] [FUNC: mls_read_user]
[LINES: 543-561]
[VAR: r]
538: goto out;
539: }
540: nel = le32_to_cpu(buf[0]);
541: l = NULL;
542: for (i = 0; i < nel; i++) {
START -->
543: r = kmalloc(sizeof(*r), GFP_ATOMIC);
544: if (!r) {
545: rc = -ENOMEM;
546: goto out;
547: }
548: memset(r, 0, sizeof(*r));
549:
550: rc = mls_read_range_helper(&r->range, fp);
551: if (rc)
GOTO -->
552: goto out;
553:
554: if (l)
555: l->next = r;
556: else
557: usrdatum->ranges = r;
558: l = r;
559: }
560:out:
END -->
561: return rc;
562:}
563:
564:int mls_read_nlevels(struct policydb *p, void *fp) 565:{
566: u32 *buf;
---------------------------------------------------------

[FILE: 2.6.0-test5/security/selinux/ss/policydb.c] [FUNC: policydb_read]
[LINES: 1232-1427]
[VAR: c]
1227: goto bad;
1228: }
1229: nel = le32_to_cpu(buf[0]);
1230: l = NULL;
1231: for (j = 0; j < nel; j++) {
START -->
1232: c = kmalloc(sizeof(*c), GFP_KERNEL);
1233: if (!c) {
1234: rc = -ENOMEM;
1235: goto bad;
1236: }

1237:			memset(c, 0, sizeof(*c));
        ... DELETED 182 lines ...

1420: }
1421: }
1422:
1423: rc = mls_read_trusted(p, fp);
1424: if (rc)
GOTO -->
1425: goto bad;
1426:out:
END -->
1427: return rc;
1428:bad:
1429: policydb_destroy(p);
1430: goto out;
---------------------------------------------------------

[FILE: 2.6.0-test5/security/selinux/ss/policydb.c] [FUNC: policydb_read]
[LINES: 1334-1427]
[VAR: newgenfs]
1329: }
1330: nel = le32_to_cpu(buf[0]);
1331: genfs_p = NULL;
1332: rc = -EINVAL;
1333: for (i = 0; i < nel; i++) {
START -->
1334: newgenfs = kmalloc(sizeof(*newgenfs), GFP_KERNEL);
1335: if (!newgenfs) {
1336: rc = -ENOMEM;
1337: goto bad;
1338: }

1339:		memset(newgenfs, 0, sizeof(*newgenfs));
        ... DELETED 5 lines ...

1345: if (!buf)
1346: goto bad;
1347: newgenfs->fstype = kmalloc(len + 1,GFP_KERNEL);
1348: if (!newgenfs->fstype) {
1349: rc = -ENOMEM;
GOTO -->
1350: goto bad;
1351: }
1352: memcpy(newgenfs->fstype, buf, len);
1353: newgenfs->fstype[len] = 0;
1354: for (genfs_p = NULL, genfs = p->genfs; genfs;
1355: genfs_p = genfs, genfs = genfs->next) {
1356: if (strcmp(newgenfs->fstype, genfs->fstype) == 0) {
1357: printk(KERN_ERR "security: dup genfs "
1358: "fstype %s\n", newgenfs->fstype);
GOTO -->
1359: goto bad;
1360: }
1361: if (strcmp(newgenfs->fstype, genfs->fstype) < 0)
1362: break;
1363: }

1364:		newgenfs->next = genfs;
        ... DELETED 57 lines ...

1422:
1423: rc = mls_read_trusted(p, fp);
1424: if (rc)
1425: goto bad;
1426:out:
END -->
1427: return rc;
1428:bad:
1429: policydb_destroy(p);
1430: goto out;
---------------------------------------------------------

[FILE: 2.6.0-test5/security/selinux/ss/policydb.c] [FUNC: policydb_read]
[LINES: 1374-1427]
[VAR: newc]
1369: buf = next_entry(fp, sizeof(u32));
1370: if (!buf)
1371: goto bad;
1372: nel2 = le32_to_cpu(buf[0]);
1373: for (j = 0; j < nel2; j++) {
START -->
1374: newc = kmalloc(sizeof(*newc), GFP_KERNEL);
1375: if (!newc) {
1376: rc = -ENOMEM;
1377: goto bad;
1378: }

1379:			memset(newc, 0, sizeof(*newc));
        ... DELETED 14 lines ...

1394: buf = next_entry(fp, sizeof(u32));
1395: if (!buf)
1396: goto bad;
1397: newc->v.sclass = le32_to_cpu(buf[0]);
1398: if (context_read_and_validate(&newc->context[0], p, fp))
GOTO -->
1399: goto bad;
1400: for (l = NULL, c = newgenfs->head; c;
1401: l = c, c = c->next) {
1402: if (!strcmp(newc->u.name, c->u.name) &&
1403: (!c->v.sclass || !newc->v.sclass ||
1404: newc->v.sclass == c->v.sclass)) {
1405: printk(KERN_ERR "security: dup genfs "
1406: "entry (%s,%s)\n",
1407: newgenfs->fstype, c->u.name);
GOTO -->
1408: goto bad;
1409: }
1410: len = strlen(newc->u.name);
1411: len2 = strlen(c->u.name);
1412: if (len > len2)

1413:					break;
        ... DELETED 8 lines ...

1422:
1423: rc = mls_read_trusted(p, fp);
1424: if (rc)
1425: goto bad;
1426:out:
END -->
1427: return rc;
1428:bad:
1429: policydb_destroy(p);
1430: goto out;
---------------------------------------------------------

[FILE: 2.6.0-test5/security/selinux/ss/policydb.c] [FUNC: class_read]
[LINES: 759-851]
[VAR: c]
754: }
755:
756: lc = NULL;
757: rc = -EINVAL;
758: for (i = 0; i < ncons; i++) {
START -->
759: c = kmalloc(sizeof(*c), GFP_KERNEL);
760: if (!c) {
761: rc = -ENOMEM;
762: goto bad;
763: }

 764:		memset(c, 0, sizeof(*c));
        ... DELETED 64 lines ...

829: c->expr = e;
830: }
831: le = e;
832: }
833: if (depth != 0)
GOTO -->
834: goto bad;
835: if (lc) {
836: lc->next = c;
837: } else {
838: cladatum->constraints = c;
839: }

        ... DELETED 6 lines ...

846:
847: rc = hashtab_insert(h, key, cladatum);
848: if (rc)
849: goto bad;
850:out:
END -->
851: return rc;
852:bad:
853: class_destroy(key, cladatum, NULL);
854: goto out;
855:}
856:
---------------------------------------------------------

[FILE: 2.6.0-test5/sound/oss/emu10k1/midi.c]
[FUNC: emu10k1_seq_midi_open]
[LINES: 498-514]
[VAR: midi_dev]
493: if (card->open_mode) /* card is opened native */
494: return -EBUSY;
495:
496: DPF(2, "emu10k1_seq_midi_open()\n");
497:
START -->
498: if ((midi_dev = (struct emu10k1_mididevice *) kmalloc(sizeof(*midi_dev), GFP_KERNEL)) == NULL)
499: return -EINVAL;
500:
501: midi_dev->card = card;
502: midi_dev->mistate = MIDIIN_STATE_STOPPED;

 503:	init_waitqueue_head(&midi_dev->oWait);
        ... DELETED 5 lines ...

509:
510: dsCardMidiOpenInfo.refdata = (unsigned long) midi_dev;
511:
512: if (emu10k1_mpuout_open(card, &dsCardMidiOpenInfo) < 0) {
513: ERROR();
END -->
514: return -ENODEV;
515: }
516:
517: card->seq_mididev = midi_dev;
518:
519: return 0;
---------------------------------------------------------

[FILE: 2.6.0-test5/sound/oss/ymfpci.c]
[FUNC: ymf_probe_one]
[LINES: 2530-2641]
[VAR: codec]
2525: printk(KERN_ERR "ymfpci: pci_enable_device failed\n");
2526: return err;
2527: }
2528: base = pci_resource_start(pcidev, 0);
2529:
START -->
2530: if ((codec = kmalloc(sizeof(ymfpci_t), GFP_KERNEL)) == NULL) {
2531: printk(KERN_ERR "ymfpci: no core\n");
2532: return -ENOMEM;
2533: }
2534: memset(codec, 0, sizeof(*codec));
2535:

        ... DELETED 11 lines ...

2547: goto out_free;
2548: }
2549:
2550: if ((codec->reg_area_virt = ioremap(base, 0x8000)) == NULL) {
2551: printk(KERN_ERR "ymfpci: unable to map registers\n");
GOTO -->
2552: goto out_release_region;
2553: }
2554:
2555: pci_set_master(pcidev);
2556:
2557: printk(KERN_INFO "ymfpci: %s at 0x%lx IRQ %d\n",

        ... DELETED 78 lines ...

2636: out_release_region:
2637: release_mem_region(pci_resource_start(pcidev, 0), 0x8000);
2638: out_free:
2639: if (codec->ac97_codec[0])
2640: ac97_release_codec(codec->ac97_codec[0]);
END -->
2641: return -ENODEV;
2642:}
2643:
2644:static void __devexit ymf_remove_one(struct pci_dev *pcidev) 2645:{
2646: __u16 ctrl;
---------------------------------------------------------

The nfsd/nfsctl.c one isn't actually a bug (though I had to think about it a bit to be sure).

One of the "... DETELED 4 lines ..." is

		file->private_data = ar;

which takes a copy of "ar" into a structure that has a lifetime greater than the function.
So when the "return -EFAULT" happends (at END -->), at is stored in file->private_data, and a subsequent call to TA_release will free that memory.

NeilBrown

Valid.

Also valid.

Invalid. This is quite an obvious false positive, at least if your algorithm checks for possible value ranges.

Valid. And not trivial to fix.

Same one, basically.

Jörn

Geld macht nicht glücklich.
Glück macht nicht satt.

But at least trivial to band-aid around it. This doesn't make the function any nicer, but it should get rid of the leaks.

David, consider this to be public domain. :)

Jörn

Mundie uses a textbook tactic of manipulation: start with some
reasonable talk, and lead the audience to an unreasonable conclusion.
-- Bruce Perens

--- linux-2.6.0-test3/fs/jffs2/scan.c~jffs2_memleak	2003-07-05 23:59:33.000000000 +0200
+++ linux-2.6.0-test3/fs/jffs2/scan.c	2003-09-16 09:16:30.000000000 +0200
@@ -106,7 +106,7 @@
 		ret = jffs2_scan_eraseblock(c, jeb, buf_size?flashbuf:(flashbuf+jeb->offset), buf_size);
 
 		if (ret < 0)
-			return ret;
+			goto out;
 
 		ACCT_PARANOIA_CHECK(jeb);
 
@@ -230,7 +230,8 @@
 		if ( !c->used_size && ((empty_blocks+bad_blocks)!= c->nr_blocks || bad_blocks == c->nr_blocks) ) {
 			printk(KERN_NOTICE "Cowardly refusing to erase blocks on filesystem with no valid JFFS2 nodes\n");
 			printk(KERN_NOTICE "empty_blocks %d, bad_blocks %d, c->nr_blocks %d\n",empty_blocks,bad_blocks,c->nr_blocks);
-			return -EIO;
+			ret = -EIO;
+			goto out;
 		}
 		jffs2_erase_pending_trigger(c);
 	}
@@ -241,6 +242,10 @@
 		c->mtd->unpoint(c->mtd, flashbuf, 0, c->mtd->size);
 #endif
 	return 0;
+out:
+	if (buf_size)
+		kfree(flashbuf);
+	return ret;
 }
 
 static int jffs2_fill_scan_buf (struct jffs2_sb_info *c, unsigned char *buf,

This should be fixed by finally taking the time and merging the command sets 0001 and 0020. Maybe someone who cares will come up with a patch.

Jörn

Simplicity is prerequisite for reliability.
-- Edsger W. Dijkstra

Actually, it *is* valid, as Wade pointed out to me.

David, please apply!

Jörn

Mundie uses a textbook tactic of manipulation: start with some
reasonable talk, and lead the audience to an unreasonable conclusion.
-- Bruce Perens

--- linux-2.6.0-test3/drivers/mtd/mtdblock.c~mtdblock_leak	2003-07-05 23:59:30.000000000 +0200
+++ linux-2.6.0-test3/drivers/mtd/mtdblock.c	2003-09-16 10:47:58.000000000 +0200
@@ -275,7 +275,7 @@
 	
 	/* OK, it's not open. Create cache info for it */
 	mtdblk = kmalloc(sizeof(struct mtdblk_dev), GFP_KERNEL);
-	if (!mtdblks)
+	if (!mtdblk)
 		return -ENOMEM;
 
 	memset(mtdblk, 0, sizeof(*mtdblk));

Wait... one is "mtdblk", and the other is "mtdblks". One has an extra 's' on it. Unless there is some kind of aliasing going on, they would appear to be different variables. Naturally, I didn't check the original code, so I could be full of it. :)

This has been resolved, see Jorn's other mail.

Yes its a bug from glancing at the source code.

On the other hand, even if the report was invalid (which another posting says isn't the case), this code is just *begging* for somebody to "fix the typo", due to how much the kernel uses

	foo = function(..)
	if (!foo)

Yeah, they're different variables - but we alloc mktblk and then return -ENOMEM if the OTHER variable is null?? ;)

[snip]

Is the attached correct?

@@ -146,8 +146,10 @@
 		/*
 		 * Attention Key.
 		 */
-		if (((ov == K_SAK) || (v == K_SAK)) && !capable(CAP_SYS_ADMIN))
+		if (((ov == K_SAK) || (v == K_SAK)) && !capable(CAP_SYS_ADMIN)) {
+			kfree(key_map);
 			return -EPERM;
+		}
 		key_map[i] = U(v);
 		if (!s && (KTYP(ov) == KT_SHIFT || KTYP(v) == KT_SHIFT))
 			compute_shiftstate();

                                ^^^^^^^^^^^^^^^^^^^^^^

Looks like there is no leak after all, as long as key_maps is dealt with properly sometime. But I'm not familiar enough with the code to judge that.

Data dominates. If you've chosen the right data structures and organized
things well, the algorithms will almost always be self-evident. Data
structures, not algorithms, are central to programming.
-- Rob Pike

No, it frees memory that is still in use, and will oops the kernel.

There was no bug to start with, so please do not fix anything.

Looks valid. And since skb isn't really needed until after these returns, moving four lines down a bit fixes the problem.

Davem, is this correct?

Jörn

Data dominates. If you've chosen the right data structures and organized
things well, the algorithms will almost always be self-evident. Data
structures, not algorithms, are central to programming.
-- Rob Pike

--- linux-2.6.0-test3/net/ipv4/igmp.c~igmp_memleak	2003-07-15 23:09:02.000000000 +0200
+++ linux-2.6.0-test3/net/ipv4/igmp.c	2003-09-16 10:29:47.000000000 +0200
@@ -70,6 +70,8 @@
  *		Alexey Kuznetsov:	Accordance to igmp-v2-06 draft.
  *		David L Stevens:	IGMPv3 support, with help from
  *					Vinay Kulkarni
+ *		Jörn Engel:		Fix memleak in igmpv3_newpack, reported
+ *					by David Yu Chen (stanford checker)
  */
 
 
@@ -271,10 +273,6 @@
 	struct iphdr *pip;
 	struct igmpv3_report *pig;
 
-	skb = alloc_skb(size + dev->hard_header_len + 15, GFP_ATOMIC);
-	if (skb == NULL)
-		return 0;
-
 	{
 		struct flowi fl = { .oif = dev->ifindex,
 				    .nl_u = { .ip4_u = {
@@ -288,6 +286,10 @@
 		return 0;
 	}
 
+	skb = alloc_skb(size + dev->hard_header_len + 15, GFP_ATOMIC);
+	if (skb == NULL)
+		return 0;
+
 	skb->dst = &rt->u.dst;
 	skb->dev = dev;

Nope, now you're leaking the route instead of the SKB :-)

I'll put the correct fix in.

[snip]

Correct?

@@ -219,8 +219,10 @@
 	instructions  = (ov->nclips + 1) *
 		((skip_even || skip_odd) ? ov->w.height>>1 :  ov->w.height);
 	instructions += 2;
-	if ((rc = btcx_riscmem_alloc(btv->dev,risc,instructions*8)) < 0)
+	if ((rc = btcx_riscmem_alloc(btv->dev,risc,instructions*8)) < 0) {
+		kfree(skips);
 		return rc;
+	}
 
 	/* sync instruction */
 	rp = risc->cpu;

[snip]

Can I ask again why the Checker is not released? I know that you don't have to release the source since you are not distributing it, but why not? AFAICS this would be a great asset to Open Source projects in general, not just the kernel.

(Patch correct? looked obvious)

@@ -511,6 +511,7 @@
 
 	if (emu10k1_mpuout_open(card, &dsCardMidiOpenInfo) < 0) {
 		ERROR();
+		kfree(midi_dev);
 		return -ENODEV;
 	}
 

There is no memory leak in the below:

Explanation:
The user may have up to 256 keymaps, namely the plain map and various maps for (combinations of) modifier keys.
In order to save memory, most of these keymaps will not be allocated. If one sets an entry on a keymap, then first the keymap itself is allocated in case it did not exist yet. Then the entry is filled, assuming one has permission.
On the error path the memory is not lost: the pointer was stored in the array key_maps[], so also am automatic checker could see that. (And indeed, it is also possible to deallocate keymaps.)

Andries

A reference to ar is stored away for kfree() on ->release():

		file->private_data = ar;

So, this will be freed when the file is closed. I don't think it's a bug.

thanks,
-chris

Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Yes, this looks like a bug. Stephen, does this fix look ok? thanks
-chris

Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net


===== security/selinux/ss/mls.c 1.1 vs edited =====
--- 1.1/security/selinux/ss/mls.c	Thu Jul 17 02:38:01 2003
+++ edited/security/selinux/ss/mls.c	Fri Sep 19 13:56:50 2003
@@ -548,8 +548,10 @@
 		memset(r, 0, sizeof(*r));
 
 		rc = mls_read_range_helper(&r->range, fp);
-		if (rc)
+		if (rc) {
+			kfree(r);
 			goto out;
+		}
 
 		if (l)
 			l->next = r;

You missed one ;-)

1344 buf = next_entry(fp, len);
1345 if (!buf)
1346 goto bad;

Yes, this is a bug.

Yes, this is a bug. I'm not sure if CHECKER caught that it leaks two allocations here. Both newgenfs and newgenfs->fstype.

You missed some more. Not sure if this is meant to be comprensive or not...

1380 buf = next_entry(fp, sizeof(u32));
1381 if (!buf)
1382 goto bad;
*HERE*
1383 len = le32_to_cpu(buf[0]);
1384 buf = next_entry(fp, len);
1385 if (!buf)
1386 goto bad;
*HERE*
1387 newc->u.name = kmalloc(len + 1,GFP_KERNEL);
1388 if (!newc->u.name) {
1389 rc = -ENOMEM;
1390 goto bad;
*HERE*

Again, not sure if you meant to capture both leakages here: newc and newc->u.name.

And both here.

And both here.

Patch below fixes the leaks in a brute force method. Stephen, look ok? I could see rearranging the code to ensure things are stored in the policydb earlier so they are all caught by policydb_destroy().

You missed this one:
765 buf = next_entry(fp, sizeof(u32)*2);
766 if (!buf)
767 goto bad;
And this one:
774 if (!e) {
775 rc = -ENOMEM;
776 goto bad;
And this one:
780 if (!buf) {
781 kfree(e);
782 goto bad;
And this one:
790 if (depth < 0) {
791 kfree(e);
792 goto bad;
etc...

Yes, this is a bug. Stephen, the patch below fixes this, and adds a constraint_destroy() function to help cleanup when the constaint_node isn't yet attached to the class_datum but there may already be some constraint_expr's on the constraint_node.

thanks,
-chris

Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net


===== security/selinux/ss/policydb.c 1.3 vs edited =====
--- 1.3/security/selinux/ss/policydb.c	Sun Aug 31 16:14:19 2003
+++ edited/security/selinux/ss/policydb.c	Fri Sep 19 15:59:02 2003
@@ -321,17 +321,11 @@
 	return 0;
 }
 
-static int class_destroy(void *key, void *datum, void *p)
+static void constraint_destroy(struct constraint_node *constraint)
 {
-	struct class_datum *cladatum;
-	struct constraint_node *constraint, *ctemp;
+	struct constraint_node *ctemp;
 	struct constraint_expr *e, *etmp;
 
-	kfree(key);
-	cladatum = datum;
-	hashtab_map(cladatum->permissions.table, perm_destroy, 0);
-	hashtab_destroy(cladatum->permissions.table);
-	constraint = cladatum->constraints;
 	while (constraint) {
 		e = constraint->expr;
 		while (e) {
@@ -344,6 +338,19 @@
 		constraint = constraint->next;
 		kfree(ctemp);
 	}
+}
+
+static int class_destroy(void *key, void *datum, void *p)
+{
+	struct class_datum *cladatum;
+	struct constraint_node *constraint;
+
+	kfree(key);
+	cladatum = datum;
+	hashtab_map(cladatum->permissions.table, perm_destroy, 0);
+	hashtab_destroy(cladatum->permissions.table);
+	constraint = cladatum->constraints;
+	constraint_destroy(constraint);
 	kfree(cladatum->comkey);
 	kfree(datum);
 	return 0;
@@ -763,8 +770,10 @@
 		}
 		memset(c, 0, sizeof(*c));
 		buf = next_entry(fp, sizeof(u32)*2);
-		if (!buf)
+		if (!buf) {
+			kfree(c);
 			goto bad;
+		}
 		c->permissions = le32_to_cpu(buf[0]);
 		nexpr = le32_to_cpu(buf[1]);
 		le = NULL;
@@ -773,11 +782,13 @@
 			e = kmalloc(sizeof(*e), GFP_KERNEL);
 			if (!e) {
 				rc = -ENOMEM;
+				constraint_destroy(c);
 				goto bad;
 			}
 			memset(e, 0, sizeof(*e));
 			buf = next_entry(fp, sizeof(u32)*3);
 			if (!buf) {
+				constraint_destroy(c);
 				kfree(e);
 				goto bad;
 			}
@@ -788,6 +799,7 @@
 			switch (e->expr_type) {
 			case CEXPR_NOT:
 				if (depth < 0) {
+					constraint_destroy(c);
 					kfree(e);
 					goto bad;
 				}
@@ -795,6 +807,7 @@
 			case CEXPR_AND:
 			case CEXPR_OR:
 				if (depth < 1) {
+					constraint_destroy(c);
 					kfree(e);
 					goto bad;
 				}
@@ -802,6 +815,7 @@
 				break;
 			case CEXPR_ATTR:
 				if (depth == (CEXPR_MAXDEPTH-1)) {
+					constraint_destroy(c);
 					kfree(e);
 					goto bad;
 				}
@@ -809,16 +823,19 @@
 				break;
 			case CEXPR_NAMES:
 				if (depth == (CEXPR_MAXDEPTH-1)) {
+					constraint_destroy(c);
 					kfree(e);
 					goto bad;
 				}
 				depth++;
 				if (ebitmap_read(&e->names, fp)) {
+					constraint_destroy(c);
 					kfree(e);
 					goto bad;
 				}
 				break;
 			default:
+				constraint_destroy(c);
 				kfree(e);
 				goto bad;
 				break;
@@ -830,8 +847,10 @@
 			}
 			le = e;
 		}
-		if (depth != 0)
+		if (depth != 0) {
+			constraint_destroy(c);
 			goto bad;
+		}
 		if (lc) {
 			lc->next = c;
 		} else {
@@ -1338,15 +1357,20 @@
 		}
 		memset(newgenfs, 0, sizeof(*newgenfs));
 		buf = next_entry(fp, sizeof(u32));
-		if (!buf)
+		if (!buf) {
+			kfree(newgenfs);
 			goto bad;
+		}
 		len = le32_to_cpu(buf[0]);
 		buf = next_entry(fp, len);
-		if (!buf)
+		if (!buf) {
+			kfree(newgenfs);
 			goto bad;
+		}
 		newgenfs->fstype = kmalloc(len + 1,GFP_KERNEL);
 		if (!newgenfs->fstype) {
 			rc = -ENOMEM;
+			kfree(newgenfs);
 			goto bad;
 		}
 		memcpy(newgenfs->fstype, buf, len);
@@ -1356,6 +1380,8 @@
 			if (strcmp(newgenfs->fstype, genfs->fstype) == 0) {
 				printk(KERN_ERR "security:  dup genfs "
 				       "fstype %s\n", newgenfs->fstype);
+				kfree(newgenfs->fstype);
+				kfree(newgenfs);
 				goto bad;
 			}
 			if (strcmp(newgenfs->fstype, genfs->fstype) < 0)
@@ -1378,25 +1404,36 @@
 			}
 			memset(newc, 0, sizeof(*newc));
 			buf = next_entry(fp, sizeof(u32));
-			if (!buf)
+			if (!buf) {
+				kfree(newc);
 				goto bad;
+			}
 			len = le32_to_cpu(buf[0]);
 			buf = next_entry(fp, len);
-			if (!buf)
+			if (!buf) {
+				kfree(newc);
 				goto bad;
+			}
 			newc->u.name = kmalloc(len + 1,GFP_KERNEL);
 			if (!newc->u.name) {
 				rc = -ENOMEM;
+				kfree(newc);
 				goto bad;
 			}
 			memcpy(newc->u.name, buf, len);
 			newc->u.name[len] = 0;
 			buf = next_entry(fp, sizeof(u32));
-			if (!buf)
+			if (!buf) {
+				kfree(newc->u.name);
+				kfree(newc);
 				goto bad;
+			}
 			newc->v.sclass = le32_to_cpu(buf[0]);
-			if (context_read_and_validate(&newc->context[0], p, fp))
+			if (context_read_and_validate(&newc->context[0], p, fp)){ 
+				kfree(newc->u.name);
+				kfree(newc);
 				goto bad;
+			}
 			for (l = NULL, c = newgenfs->head; c;
 			     l = c, c = c->next) {
 				if (!strcmp(newc->u.name, c->u.name) &&
@@ -1405,6 +1442,8 @@
 					printk(KERN_ERR "security:  dup genfs "
 					       "entry (%s,%s)\n",
 					       newgenfs->fstype, c->u.name);
+					kfree(newc->u.name);
+					kfree(newc);
 					goto bad;
 				}
 				len = strlen(newc->u.name);

Hi Chris,

Sorry for the duplicated effort, but I had already written up a patch prior to the hurricane, but didn't get it sent out. I believe that the patch below fixes the legitimate leaks in the SELinux code. In some cases, it rearranges the code (moving the allocation later to reduce the need for further cleanup or linking the object into a containing structure earlier so that the policydb_destroy will handle it upon any subsequent errors).

 security/selinux/ss/mls.c      |   13 ++++
 security/selinux/ss/policydb.c |  112 ++++++++++++++++++++---------------------
 2 files changed, 68 insertions(+), 57 deletions(-)

Index: linux-2.6/security/selinux/ss/mls.c
===================================================================
RCS file: /nfshome/pal/CVS/linux-2.6/security/selinux/ss/mls.c,v retrieving revision 1.16

diff -u -r1.16 mls.c
@@ -548,8 +548,10 @@
 		memset(r, 0, sizeof(*r));
 
 		rc = mls_read_range_helper(&r->range, fp);
-		if (rc)
+		if (rc) {
+			kfree(r);
 			goto out;
+		}
 
 		if (l)
 			l->next = r;

@@ -581,10 +583,17 @@
 		goto out;
 	rc = ebitmap_read(&p->trustedwriters, fp);
 	if (rc)
-		goto out;
+		goto bad;
 	rc = ebitmap_read(&p->trustedobjects, fp);
+	if (rc)
+		goto bad2;
 out:
 	return rc;
+bad2:
+	ebitmap_destroy(&p->trustedwriters);
+bad:
+	ebitmap_destroy(&p->trustedreaders);
+	goto out;
 }
 
 int sens_index(void *key, void *datum, void *datap)

Index: linux-2.6/security/selinux/ss/policydb.c
===================================================================
RCS file: /nfshome/pal/CVS/linux-2.6/security/selinux/ss/policydb.c,v retrieving revision 1.24

Stephen Smalley <[email protected]>
National Security Agency

No problem, your patch looks better anyway ;-)
-chris

Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

You missed a reference to the context is stored in the policydb:

			p->ocontexts[i] = c;

The policydb is destroyed on error, which will free the context. I don't think this is a bug.

thanks,
-chris

Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Yes, this could leak sdbg_devinfo structs. Patch below. James, look ok?

thanks,
-chris

===== drivers/scsi/scsi_debug.c 1.44 vs edited =====

@@ -1614,7 +1614,7 @@
                         printk(KERN_ERR "%s: out of memory at line %d\n",
                                __FUNCTION__, __LINE__);
                         error = -ENOMEM;
-			goto clean1;
+			goto clean;
                 }
                 memset(sdbg_devinfo, 0, sizeof(*sdbg_devinfo));
                 sdbg_devinfo->sdbg_host = sdbg_host;

@@ -1634,12 +1634,12 @@
         error = device_register(&sdbg_host->dev);
 
         if (error)
-		goto clean2;
+		goto clean;
 
 	++scsi_debug_add_host;
         return error;
 
-clean2:
+clean:
 	list_for_each_safe(lh, lh_sf, &sdbg_host->dev_info_list) {
 		sdbg_devinfo = list_entry(lh, struct sdebug_dev_info,
 					  dev_list);

@@ -1647,7 +1647,6 @@
 		kfree(sdbg_devinfo);
 	}
 
-clean1:
 	kfree(sdbg_host);
         return error;
 }

<snip>

Yes, looks like a valid bug. Patch below. James, look ok? thanks,
-chris

===== drivers/scsi/NCR_Q720.c 1.4 vs edited =====

@@ -179,7 +179,7 @@
 	i = inb(io_base) | (inb(io_base+1)<<8);
 	if(i != NCR_Q720_MCA_ID) {
 		printk(KERN_ERR "NCR_Q720, adapter failed to I/O map registers correctly at 0x%x(0x%x)\n", io_base, i);
-		return -ENODEV;
+		goto out_free;
 	}
 
 	/* Phase II, find the ram base and memory map the board register */

Yes, this is a bug. Patch below.

thanks,
-chris

===== drivers/telephony/ixj_pcmcia.c 1.6 vs edited =====

@@ -60,8 +60,10 @@
 	link->conf.Vcc = 50;
 	link->conf.IntType = INT_MEMORY_AND_IO;
 	link->priv = kmalloc(sizeof(struct ixj_info_t), GFP_KERNEL);
-	if (!link->priv)
+	if (!link->priv) {
+		kfree(link);
 		return NULL;
+	}
 	memset(link->priv, 0, sizeof(struct ixj_info_t));
 	/* Register with Card Services */
 	link->next = dev_list;

<snip>

<snip>

<snip>

<snip>

Yes, these are bugs. Patch below. Greg, this look ok?

thanks,
-chris

===== drivers/usb/class/usb-midi.c 1.22 vs edited =====

@@ -1750,7 +1750,7 @@
 	return 0;
 
  error_end:
-	if ( mdevs != NULL && devices > 0 ) {
+	if ( mdevs != NULL ) {
 		for ( i=0 ; i<devices ; i++ ) {
 			if ( mdevs[i] != NULL ) {
 				unregister_sound_midi( mdevs[i]->dev_midi );

Don't know, Vojtech said he would fix these up already. Try asking him :)

thanks,

greg k-h

Ah, I missed that. Will do.
thanks,
-chris

Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

I checked with Vojtech, he said the patch looked OK. Can you apply?

thanks,
-chris

===== drivers/usb/class/usb-midi.c 1.22 vs edited =====

@@ -1750,7 +1750,7 @@
 	return 0;
 
  error_end:
-	if ( mdevs != NULL && devices > 0 ) {
+	if ( mdevs != NULL ) {
 		for ( i=0 ; i<devices ; i++ ) {
 			if ( mdevs[i] != NULL ) {
 				unregister_sound_midi( mdevs[i]->dev_midi );

Applied, thanks.

greg k-h

<snip>

Yes, this looks like a bug/typo. Patch below. David, this look ok?

thanks,
-chris

===== fs/afs/cell.c 1.2 vs edited =====

@@ -145,7 +145,7 @@
 	printk("kAFS: bad VL server IP address: '%s'\n",vllist);
  error:
 	up_write(&afs_cells_sem);
-	kfree(afs_cell_root);
+	kfree(cell);
 	return ret;
 } /* end afs_cell_create() */
 

Yes. I've applied it to my local development version.

Thanks,
David

<snip>

Yes, these are both bugs. Patch below. Trond, this look ok?

thanks,
-chris

===== net/sunrpc/auth_gss/gss_mech_switch.c 1.2 vs edited =====

@@ -71,6 +71,7 @@
 	gm->gm_oid.len = mech_type->len;
 	if (!(gm->gm_oid.data = kmalloc(mech_type->len, GFP_KERNEL))) {
 		printk("Failed to allocate memory in gss_mech_register");
+		kfree(gm);
 		return -1;
 	}
 	memcpy(gm->gm_oid.data, mech_type->data, mech_type->len);

===== net/sunrpc/auth_gss/gss_pseudoflavors.c 1.1 vs edited =====

@@ -93,6 +93,7 @@
 
 err_unlock:
 	spin_unlock(&registered_triples_lock);
+	kfree(triple);
 err:
 	return -1;
 }

Yes, this is a bug. Patch below. Jean, look ok?

thanks,
-chris

===== net/irda/irlmp.c 1.29 vs edited =====

@@ -178,7 +178,7 @@
 
 	init_timer(&self->watchdog_timer);
 
-	ASSERT(notify->instance != NULL, return NULL;);
+	ASSERT(notify->instance != NULL, goto error;);
 	self->notify = *notify;
 
 	self->lsap_state = LSAP_DISCONNECTED;

@@ -188,6 +188,9 @@
 		       (long) self, NULL);
 
 	return self;
+error:
+	kfree(self);
+	return NULL;
 }
 
 /*

Yep, looks like the right thing. But, I would prefer a slighly
different fix. Would you mind moving the assert at the top of the function ?
Just like this :
--------------------------------------------------------------------

@@ -146,6 +146,7 @@ struct lsap_cb *irlmp_open_lsap(__u8 sls
        ASSERT(notify != NULL, return NULL;);
        ASSERT(irlmp != NULL, return NULL;);
        ASSERT(irlmp->magic == LMP_MAGIC, return NULL;);
+       ASSERT(notify->instance != NULL, return NULL;);
 
        /*  Does the client care which Source LSAP selector it gets?  */
        if (slsap_sel == LSAP_ANY) {

@@ -178,7 +179,6 @@ struct lsap_cb *irlmp_open_lsap(__u8 sls
 
        init_timer(&self->watchdog_timer);
 
-       ASSERT(notify->instance != NULL, return NULL;);
        self->notify = *notify;
 
        self->lsap_state = LSAP_DISCONNECTED;

--------------------------------------------------------------------

	This is simpler and more efficient ;-)

	Thanks...

	Jean

Yes, I agree, your patch is better.

-thanks,
-chris

Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Yes, this looks like a bug. Patch below. Alan, this look ok?

thanks,
-chris

===== sound/oss/ymfpci.c 1.38 vs edited =====

@@ -2638,6 +2638,7 @@
  out_free:
 	if (codec->ac97_codec[0])
 		ac97_release_codec(codec->ac97_codec[0]);
+	kfree(codec);
 	return -ENODEV;
 }