lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 17 Nov 2013 23:12:38 -0200 From: Geyslan Gregório Bem <geyslan@...il.com> To: James Bottomley <James.Bottomley@...senpartnership.com> Cc: Jayamohan Kallickal <jayamohan.kallickal@...lex.com>, "open list:SERVER ENGINES 10..." <linux-scsi@...r.kernel.org>, open list <linux-kernel@...r.kernel.org> Subject: Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code 2013/11/17 James Bottomley <James.Bottomley@...senpartnership.com>: > On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote: >> 2013/11/17 James Bottomley <James.Bottomley@...senpartnership.com>: >> > On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote: >> >> This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and >> >> 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when >> >> necessary. >> > >> > You pointlessly renamed a variable, which makes the diff hard to read. >> > Please don't do that. >> >> Ok, I can agree. 'len' means length? What is returned in case of non >> error? > > it returns the length of buf written to or negative error. > >> > You missed the fact that the passed in pointer is unmodified if >> > mgmt_get_if_info() returns non zero, so the kfree frees junk and would >> > oops. >> > >> > There's no need for a goto; len = -EINVAL; does everything that's >> > needed. >> >> Well, that is a coverity catch. CID 1128954. Check it. > > I didn't say coverity was wrong, I said your patch was (well not wrong, > just over complex and incomplete). This is the way to fix both > problems. > > James > > --- > > diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c > index ffadbee..9dcbdfa 100644 > --- a/drivers/scsi/be2iscsi/be_iscsi.c > +++ b/drivers/scsi/be2iscsi/be_iscsi.c > @@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba, > ip_type = BE2_IPV6; James, this approach will not prevent the leakage. We can initialize the if_info with NULL and always kfree it without to care about junk. > > len = mgmt_get_if_info(phba, ip_type, &if_info); > - if (len) { > - kfree(if_info); > + if (len) > return len; > - } > > switch (param) { > case ISCSI_NET_PARAM_IPV4_ADDR: > @@ -569,7 +567,7 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba, > break; > case ISCSI_NET_PARAM_VLAN_ID: > if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE) > - return -EINVAL; > + len = -EINVAL; > else > len = sprintf(buf, "%d\n", > (if_info->vlan_priority & > > -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists