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
| ||
|
Message-ID: <47207E5E.80305@linux.vnet.ibm.com> Date: Thu, 25 Oct 2007 17:00:38 +0530 From: Balbir Singh <balbir@...ux.vnet.ibm.com> To: Adrian Bunk <bunk@...nel.org> CC: linux-kernel@...r.kernel.org Subject: Re: [2.6 patch] kernel/taskstats.c: fix bogus nlmsg_free() Adrian Bunk wrote: > I considered it more odd to read > > > rc = cgroupstats_build(stats, file->f_dentry); > if (rc < 0) > goto err; > > fput_light(file, fput_needed); > return send_reply(rep_skb, info->snd_pid); > err: > fput_light(file, fput_needed); > nlmsg_free(rep_skb); > I agree, but sometimes the alternative can be worse to read and understand > > But that's just personal taste. > > Anyway, duplicating the same fput_light() call two or three times isn't > nice. > > Originally I had the bigger patch below and considered it too big for > only this fix, but now I think it might be appropriate. > > If you ignore whitespace when diff'ing, then all it does is: > > <-- snip --> > > @@ -398,7 +398,9 @@ static int cgroupstats_user_cmd(struct sk_buff *skb, struct genl_info *info) > > fd = nla_get_u32(info->attrs[CGROUPSTATS_CMD_ATTR_FD]); > file = fget_light(fd, &fput_needed); > - if (file) { > + if (!file) > + return 0; > + > size = nla_total_size(sizeof(struct cgroupstats)); > > rc = prepare_reply(info, CGROUPSTATS_CMD_NEW, &rep_skb, > @@ -412,17 +414,15 @@ static int cgroupstats_user_cmd(struct sk_buff *skb, struct genl_info *info) > memset(stats, 0, sizeof(*stats)); > > rc = cgroupstats_build(stats, file->f_dentry); > - if (rc < 0) > + if (rc < 0) { > + nlmsg_free(rep_skb); > goto err; > - > - fput_light(file, fput_needed); > - return send_reply(rep_skb, info->snd_pid); > } > > + rc = send_reply(rep_skb, info->snd_pid); > + > err: > - if (file) > fput_light(file, fput_needed); > - nlmsg_free(rep_skb); > return rc; > } > > <-- snip --> > > > Is this patch OK for you? > > > cu > Adrian > > > <-- snip --> > > > We'd better not nlmsg_free on a pointer containing an undefined value > (and without having anything allocated). > > Spotted by the Coverity checker. > > Signed-off-by: Adrian Bunk <bunk@...nel.org> > > --- > > kernel/taskstats.c | 36 ++++++++++++++++++------------------ > 1 file changed, 18 insertions(+), 18 deletions(-) > > eedd297ac36b5d92fc38b937677ba40dc6df1cd0 > diff --git a/kernel/taskstats.c b/kernel/taskstats.c > index 354e74b..07e86a8 100644 > --- a/kernel/taskstats.c > +++ b/kernel/taskstats.c > @@ -398,31 +398,31 @@ static int cgroupstats_user_cmd(struct sk_buff *skb, struct genl_info *info) > > fd = nla_get_u32(info->attrs[CGROUPSTATS_CMD_ATTR_FD]); > file = fget_light(fd, &fput_needed); > - if (file) { > - size = nla_total_size(sizeof(struct cgroupstats)); > + if (!file) > + return 0; > > - rc = prepare_reply(info, CGROUPSTATS_CMD_NEW, &rep_skb, > - size); > - if (rc < 0) > - goto err; > + size = nla_total_size(sizeof(struct cgroupstats)); > > - na = nla_reserve(rep_skb, CGROUPSTATS_TYPE_CGROUP_STATS, > - sizeof(struct cgroupstats)); > - stats = nla_data(na); > - memset(stats, 0, sizeof(*stats)); > + rc = prepare_reply(info, CGROUPSTATS_CMD_NEW, &rep_skb, > + size); > + if (rc < 0) > + goto err; > > - rc = cgroupstats_build(stats, file->f_dentry); > - if (rc < 0) > - goto err; > + na = nla_reserve(rep_skb, CGROUPSTATS_TYPE_CGROUP_STATS, > + sizeof(struct cgroupstats)); > + stats = nla_data(na); > + memset(stats, 0, sizeof(*stats)); > > - fput_light(file, fput_needed); > - return send_reply(rep_skb, info->snd_pid); > + rc = cgroupstats_build(stats, file->f_dentry); > + if (rc < 0) { > + nlmsg_free(rep_skb); > + goto err; > } > > + rc = send_reply(rep_skb, info->snd_pid); > + > err: > - if (file) > - fput_light(file, fput_needed); > - nlmsg_free(rep_skb); > + fput_light(file, fput_needed); > return rc; > } > > Looks good to me! Acked-by: Balbir Singh <balbir@...ux.vnet.ibm> Yet to be tested, I'll try and test it soon. -- Warm Regards, Balbir Singh Linux Technology Center IBM, ISTL - 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