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: <0f6ebc50-06d6-4e3d-b296-1045b0255c8a@gmail.com> Date: Fri, 27 Oct 2023 10:41:08 +0800 From: Hangyu Hua <hbh25y@...il.com> To: asmadeus@...ewreck.org Cc: ericvh@...nel.org, lucho@...kov.net, linux_oss@...debyte.com, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, jvrao@...ux.vnet.ibm.com, v9fs@...ts.linux.dev, netdev@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH] net: 9p: fix possible memory leak in p9_check_errors() On 26/10/2023 19:53, asmadeus@...ewreck.org wrote: > > Hangyu Hua wrote on Thu, Oct 26, 2023 at 05:23:51PM +0800: >> When p9pdu_readf is called with "s?d" attribute, it allocates a pointer >> that will store a string. But when p9pdu_readf() fails while handling "d" >> then this pointer will not be freed in p9_check_errors. > > Right, that sounds correct to me. > > Out of curiosity how did you notice this? The leak shouldn't happen with > any valid server. I just found that any attributes that require memory allocation are prone to errors when mixed with ordinary attributes. > > This cannot break anything so I'll push this to -next tomorrow and > submit to Linus next week Agreed. > >> Fixes: ca41bb3e21d7 ("[net/9p] Handle Zero Copy TREAD/RERROR case in !dotl case.") > > This commit moves this code a bit, but the p9pdu_readf call predates > it -- in this case the Fixes tag is probably not useful; this affects > all maintained kernels. > >> Signed-off-by: Hangyu Hua <hbh25y@...il.com> >> --- >> net/9p/client.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/net/9p/client.c b/net/9p/client.c >> index 86bbc7147fc1..6c7cd765b714 100644 >> --- a/net/9p/client.c >> +++ b/net/9p/client.c >> @@ -540,12 +540,15 @@ static int p9_check_errors(struct p9_client *c, struct p9_req_t *req) >> return 0; >> >> if (!p9_is_proto_dotl(c)) { >> - char *ename; >> + char *ename = NULL; >> >> err = p9pdu_readf(&req->rc, c->proto_version, "s?d", >> &ename, &ecode); >> - if (err) >> + if (err) { >> + if (ename != NULL) >> + kfree(ename); > > Don't check for NULL before kfree - kfree does it. > If that's the only remark you get I can fix it when applying the commit > on my side. I get it. I will revise it based on your and Christian's comments and send a v2. Thanks, Hangyu > > >> goto out_err; >> + } >> >> if (p9_is_proto_dotu(c) && ecode < 512) >> err = -ecode; >
Powered by blists - more mailing lists