| Message ID | 20231206200913.16135-1-pchelkin@ispras.ru (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v4] net: 9p: avoid freeing uninit memory in p9pdu_vreadf | expand |
On Wednesday, December 6, 2023 9:09:13 PM CET Fedor Pchelkin wrote: > If some of p9pdu_readf() calls inside case 'T' in p9pdu_vreadf() fails, > the error path is not handled properly. *wnames or members of *wnames > array may be left uninitialized and invalidly freed. > > Initialize *wnames to NULL in beginning of case 'T'. Initialize the first > *wnames array element to NULL and nullify the failing *wnames element so > that the error path freeing loop stops on the first NULL element and > doesn't proceed further. > > Found by Linux Verification Center (linuxtesting.org). > > Fixes: ace51c4dd2f9 ("9p: add new protocol support code") > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> > --- > v2: I've missed that *wnames can also be left uninitialized. Please > ignore the patch v1. As an answer to Dominique's comment: my > organization marks this statement in all commits. > v3: Simplify the patch by using kcalloc() instead of array indices > manipulation per Christian Schoenebeck's remark. Update the commit > message accordingly. > v4: Per Christian's suggestion, apply another strategy: mark failing > array element as NULL and move in the freeing loop until it is found. > Update the commit message accordingly. If v4 is more appropriate than the > version at > https://github.com/martinetd/linux/commit/69cc23eb3a0b79538e9b5face200c4cd5cd32ae0 > then please use it, otherwise, I don't think we can provide more > convenient solution here than the one already queued at github. > > net/9p/protocol.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/net/9p/protocol.c b/net/9p/protocol.c > index 4e3a2a1ffcb3..0e6603b1ec90 100644 > --- a/net/9p/protocol.c > +++ b/net/9p/protocol.c > @@ -394,6 +394,8 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > uint16_t *nwname = va_arg(ap, uint16_t *); > char ***wnames = va_arg(ap, char ***); > > + *wnames = NULL; > + > errcode = p9pdu_readf(pdu, proto_version, > "w", nwname); > if (!errcode) { > @@ -403,6 +405,8 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > GFP_NOFS); > if (!*wnames) > errcode = -ENOMEM; > + else > + (*wnames)[0] = NULL; > } > > if (!errcode) { > @@ -414,8 +418,10 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > proto_version, > "s", > &(*wnames)[i]); > - if (errcode) > + if (errcode) { > + (*wnames)[i] = NULL; > break; > + } I just checked whether this could create a leak, but it looks clean, so LGTM: Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com> Dominique, I would tend to use this v4 instead of v2. What do you think? > } > } > > @@ -423,11 +429,14 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > if (*wnames) { > int i; > > - for (i = 0; i < *nwname; i++) > + for (i = 0; i < *nwname; i++) { > + if (!(*wnames)[i]) > + break; > kfree((*wnames)[i]); > + } > + kfree(*wnames); > + *wnames = NULL; > } > - kfree(*wnames); > - *wnames = NULL; > } > } > break; >
On Wed, Dec 06, 2023 at 11:09:13PM +0300, Fedor Pchelkin wrote: > If some of p9pdu_readf() calls inside case 'T' in p9pdu_vreadf() fails, > the error path is not handled properly. *wnames or members of *wnames > array may be left uninitialized and invalidly freed. > > Initialize *wnames to NULL in beginning of case 'T'. Initialize the first > *wnames array element to NULL and nullify the failing *wnames element so > that the error path freeing loop stops on the first NULL element and > doesn't proceed further. > > Found by Linux Verification Center (linuxtesting.org). > > Fixes: ace51c4dd2f9 ("9p: add new protocol support code") > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> Reviewed-by: Simon Horman <horms@kernel.org>
Christian Schoenebeck wrote on Thu, Dec 07, 2023 at 01:54:02PM +0100:
> I just checked whether this could create a leak, but it looks clean, so LGTM:
Right, either version look good to me.
I don't have a hard preference here, I've finished testing and just
updated the patch -- thanks for your comments & review
(and thanks Simon as well!)
Dominique, On Tue, Dec 12, 2023 at 08:21:30AM +0900, Dominique Martinet wrote: > Christian Schoenebeck wrote on Thu, Dec 07, 2023 at 01:54:02PM +0100: > > I just checked whether this could create a leak, but it looks clean, so LGTM: > > Right, either version look good to me. Also, there was unnoticed bug in v2[1] - `int i` is moved to outer block and `i` counld be used uninitialized inside of `if (errcode) {`. Thanks, [1] https://lore.kernel.org/all/20231205091952.24754-1-pchelkin@ispras.ru/ > I don't have a hard preference here, I've finished testing and just > updated the patch -- thanks for your comments & review > (and thanks Simon as well!) > > -- > Dominique Martinet | Asmadeus
On 24/01/07 10:56AM, Vitaly Chikunov wrote: > Dominique, > > On Tue, Dec 12, 2023 at 08:21:30AM +0900, Dominique Martinet wrote: > > Christian Schoenebeck wrote on Thu, Dec 07, 2023 at 01:54:02PM +0100: > > > I just checked whether this could create a leak, but it looks clean, so LGTM: > > > > Right, either version look good to me. > > Also, there was unnoticed bug in v2[1] - `int i` is moved to outer block > and `i` counld be used uninitialized inside of `if (errcode) {`. Could you elaborate, please? As I can see, `i` could be used uninitialized in `if (errcode) {` only when `*wnames` is not NULL. But when `*wnames` is not NULL, then `i` is initialized in the `for` loop. It is a bit tricky and not obvious from the first glance (and not the best decision after all), so with Christian's advice the patch was rewritten to v4 which was eventually accepted. The bug you've noticed exists in v1 of the patch, not v2. > Thanks, > > [1] https://lore.kernel.org/all/20231205091952.24754-1-pchelkin@ispras.ru/
Fedor, On Sun, Jan 07, 2024 at 12:48:11PM +0300, Fedor Pchelkin wrote: > On 24/01/07 10:56AM, Vitaly Chikunov wrote: > > > > On Tue, Dec 12, 2023 at 08:21:30AM +0900, Dominique Martinet wrote: > > > Christian Schoenebeck wrote on Thu, Dec 07, 2023 at 01:54:02PM +0100: > > > > I just checked whether this could create a leak, but it looks clean, so LGTM: > > > > > > Right, either version look good to me. > > > > Also, there was unnoticed bug in v2[1] - `int i` is moved to outer block > > and `i` counld be used uninitialized inside of `if (errcode) {`. > > Could you elaborate, please? As I can see, `i` could be used > uninitialized in `if (errcode) {` only when `*wnames` is not NULL. But > when `*wnames` is not NULL, then `i` is initialized in the `for` loop. It > is a bit tricky and not obvious from the first glance (and not the best > decision after all), so with Christian's advice the patch was rewritten > to v4 which was eventually accepted. > > The bug you've noticed exists in v1 of the patch, not v2. You are right, it only affects v1. I didn't notice that important difference in v2. My excuses! Thanks, > > > Thanks, > > > > [1] https://lore.kernel.org/all/20231205091952.24754-1-pchelkin@ispras.ru/
Vitaly Chikunov wrote on Sun, Jan 07, 2024 at 10:56:23AM +0300: > On Tue, Dec 12, 2023 at 08:21:30AM +0900, Dominique Martinet wrote: > > Christian Schoenebeck wrote on Thu, Dec 07, 2023 at 01:54:02PM +0100: > > > I just checked whether this could create a leak, but it looks clean, so LGTM: > > > > Right, either version look good to me. > > Also, there was unnoticed bug in v2[1] - `int i` is moved to outer block > and `i` counld be used uninitialized inside of `if (errcode) {`. Thanks for pointing it out; I'm not sure if it's the same thing Christian noticed but I believe what I had applied with his suggestion of initializing i worked either way... But this is moot since I updated to v4 afterwards: this v4 has been merged in 6.7-rc7 as follow: https://git.kernel.org/linus/ff49bf1867578f23a5ffdd38f927f6e1e16796c4 (the message-id also points to the v4 correctly, e.g. https://lkml.kernel.org/r/20231206200913.16135-1-pchelkin@ispras.ru ) Thanks,
diff --git a/net/9p/protocol.c b/net/9p/protocol.c index 4e3a2a1ffcb3..0e6603b1ec90 100644 --- a/net/9p/protocol.c +++ b/net/9p/protocol.c @@ -394,6 +394,8 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, uint16_t *nwname = va_arg(ap, uint16_t *); char ***wnames = va_arg(ap, char ***); + *wnames = NULL; + errcode = p9pdu_readf(pdu, proto_version, "w", nwname); if (!errcode) { @@ -403,6 +405,8 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, GFP_NOFS); if (!*wnames) errcode = -ENOMEM; + else + (*wnames)[0] = NULL; } if (!errcode) { @@ -414,8 +418,10 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, proto_version, "s", &(*wnames)[i]); - if (errcode) + if (errcode) { + (*wnames)[i] = NULL; break; + } } } @@ -423,11 +429,14 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, if (*wnames) { int i; - for (i = 0; i < *nwname; i++) + for (i = 0; i < *nwname; i++) { + if (!(*wnames)[i]) + break; kfree((*wnames)[i]); + } + kfree(*wnames); + *wnames = NULL; } - kfree(*wnames); - *wnames = NULL; } } break;
If some of p9pdu_readf() calls inside case 'T' in p9pdu_vreadf() fails, the error path is not handled properly. *wnames or members of *wnames array may be left uninitialized and invalidly freed. Initialize *wnames to NULL in beginning of case 'T'. Initialize the first *wnames array element to NULL and nullify the failing *wnames element so that the error path freeing loop stops on the first NULL element and doesn't proceed further. Found by Linux Verification Center (linuxtesting.org). Fixes: ace51c4dd2f9 ("9p: add new protocol support code") Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> --- v2: I've missed that *wnames can also be left uninitialized. Please ignore the patch v1. As an answer to Dominique's comment: my organization marks this statement in all commits. v3: Simplify the patch by using kcalloc() instead of array indices manipulation per Christian Schoenebeck's remark. Update the commit message accordingly. v4: Per Christian's suggestion, apply another strategy: mark failing array element as NULL and move in the freeing loop until it is found. Update the commit message accordingly. If v4 is more appropriate than the version at https://github.com/martinetd/linux/commit/69cc23eb3a0b79538e9b5face200c4cd5cd32ae0 then please use it, otherwise, I don't think we can provide more convenient solution here than the one already queued at github. net/9p/protocol.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)