| Message ID | 20231026092351.30572-1-hbh25y@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: 9p: fix possible memory leak in p9_check_errors() | expand |
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. This cannot break anything so I'll push this to -next tomorrow and submit to Linus next week > 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. > goto out_err; > + } > > if (p9_is_proto_dotu(c) && ecode < 512) > err = -ecode;
On Thursday, October 26, 2023 1:53:55 PM CEST 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. > > This cannot break anything so I'll push this to -next tomorrow and > submit to Linus next week > > > 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. Looks like it exists since introduction of p9_check_errors(), therefore: Fixes: 51a87c552dfd ("9p: rework client code to use new protocol support functions") > > 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. With those two remarks addressed: Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com> > > > > goto out_err; > > + } > > > > if (p9_is_proto_dotu(c) && ecode < 512) > > err = -ecode; > >
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; >
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); goto out_err; + } if (p9_is_proto_dotu(c) && ecode < 512) err = -ecode;
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. Fixes: ca41bb3e21d7 ("[net/9p] Handle Zero Copy TREAD/RERROR case in !dotl case.") Signed-off-by: Hangyu Hua <hbh25y@gmail.com> --- net/9p/client.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)