Messages in this thread | | | Date | Fri, 27 Oct 2023 10:41:08 +0800 | Subject | Re: [PATCH] net: 9p: fix possible memory leak in p9_check_errors() | From | Hangyu Hua <> |
| |
On 26/10/2023 19:53, asmadeus@codewreck.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@gmail.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; >
| |