* [PATCH] net: 9p: avoid freeing uninit memory in p9pdu_vreadf @ 2023-12-05 8:05 Fedor Pchelkin 2023-12-05 9:07 ` Dominique Martinet 0 siblings, 1 reply; 18+ messages in thread From: Fedor Pchelkin @ 2023-12-05 8:05 UTC (permalink / raw) To: Eric Van Hensbergen Cc: Fedor Pchelkin, Latchesar Ionkov, Dominique Martinet, Christian Schoenebeck, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project If an error occurs while processing an array of strings in p9pdu_vreadf then uninitialized members of *wnames array are freed. Fix this by iterating over only lower indices of the array. Found by Linux Verification Center (linuxtesting.org). Fixes: ace51c4dd2f9 ("9p: add new protocol support code") Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> --- net/9p/protocol.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/net/9p/protocol.c b/net/9p/protocol.c index 4e3a2a1ffcb3..d33387e74a66 100644 --- a/net/9p/protocol.c +++ b/net/9p/protocol.c @@ -393,6 +393,7 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, case 'T':{ uint16_t *nwname = va_arg(ap, uint16_t *); char ***wnames = va_arg(ap, char ***); + int i; errcode = p9pdu_readf(pdu, proto_version, "w", nwname); @@ -406,8 +407,6 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, } if (!errcode) { - int i; - for (i = 0; i < *nwname; i++) { errcode = p9pdu_readf(pdu, @@ -421,9 +420,7 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, if (errcode) { if (*wnames) { - int i; - - for (i = 0; i < *nwname; i++) + while (--i >= 0) kfree((*wnames)[i]); } kfree(*wnames); -- 2.43.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2023-12-05 8:05 [PATCH] net: 9p: avoid freeing uninit memory in p9pdu_vreadf Fedor Pchelkin @ 2023-12-05 9:07 ` Dominique Martinet 2023-12-05 9:19 ` [PATCH v2] " Fedor Pchelkin 0 siblings, 1 reply; 18+ messages in thread From: Dominique Martinet @ 2023-12-05 9:07 UTC (permalink / raw) To: Fedor Pchelkin Cc: Eric Van Hensbergen, Latchesar Ionkov, Christian Schoenebeck, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project Fedor Pchelkin wrote on Tue, Dec 05, 2023 at 11:05:22AM +0300: > If an error occurs while processing an array of strings in p9pdu_vreadf > then uninitialized members of *wnames array are freed. > > Fix this by iterating over only lower indices of the array. > > Found by Linux Verification Center (linuxtesting.org). You might want to mark that as Reported-by: somehow instead of a free form comment > > Fixes: ace51c4dd2f9 ("9p: add new protocol support code") > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> That aside, it looks good to me -- good find! I'll push this to Linus with the other pending fix we have next week > --- > net/9p/protocol.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/net/9p/protocol.c b/net/9p/protocol.c > index 4e3a2a1ffcb3..d33387e74a66 100644 > --- a/net/9p/protocol.c > +++ b/net/9p/protocol.c > @@ -393,6 +393,7 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > case 'T':{ > uint16_t *nwname = va_arg(ap, uint16_t *); > char ***wnames = va_arg(ap, char ***); > + int i; > > errcode = p9pdu_readf(pdu, proto_version, > "w", nwname); > @@ -406,8 +407,6 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > } > > if (!errcode) { > - int i; > - > for (i = 0; i < *nwname; i++) { > errcode = > p9pdu_readf(pdu, > @@ -421,9 +420,7 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > > if (errcode) { > if (*wnames) { > - int i; > - > - for (i = 0; i < *nwname; i++) > + while (--i >= 0) > kfree((*wnames)[i]); > } > kfree(*wnames); -- Dominique Martinet | Asmadeus ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2023-12-05 9:07 ` Dominique Martinet @ 2023-12-05 9:19 ` Fedor Pchelkin 2023-12-05 9:31 ` Dominique Martinet 2023-12-05 12:29 ` Christian Schoenebeck 0 siblings, 2 replies; 18+ messages in thread From: Fedor Pchelkin @ 2023-12-05 9:19 UTC (permalink / raw) To: Dominique Martinet Cc: Fedor Pchelkin, Latchesar Ionkov, Eric Van Hensbergen, Christian Schoenebeck, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project If an error occurs while processing an array of strings in p9pdu_vreadf then uninitialized members of *wnames array are freed. Fix this by iterating over only lower indices of the array. Also handle possible uninit *wnames usage if first p9pdu_readf() call inside 'T' case fails. 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. net/9p/protocol.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/net/9p/protocol.c b/net/9p/protocol.c index 4e3a2a1ffcb3..043b621f8b84 100644 --- a/net/9p/protocol.c +++ b/net/9p/protocol.c @@ -393,6 +393,8 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, case 'T':{ uint16_t *nwname = va_arg(ap, uint16_t *); char ***wnames = va_arg(ap, char ***); + int i; + *wnames = NULL; errcode = p9pdu_readf(pdu, proto_version, "w", nwname); @@ -406,8 +408,6 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, } if (!errcode) { - int i; - for (i = 0; i < *nwname; i++) { errcode = p9pdu_readf(pdu, @@ -421,13 +421,11 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, if (errcode) { if (*wnames) { - int i; - - for (i = 0; i < *nwname; i++) + while (--i >= 0) kfree((*wnames)[i]); + kfree(*wnames); + *wnames = NULL; } - kfree(*wnames); - *wnames = NULL; } } break; -- 2.43.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2023-12-05 9:19 ` [PATCH v2] " Fedor Pchelkin @ 2023-12-05 9:31 ` Dominique Martinet 2023-12-05 12:15 ` Fedor Pchelkin 2023-12-05 12:29 ` Christian Schoenebeck 1 sibling, 1 reply; 18+ messages in thread From: Dominique Martinet @ 2023-12-05 9:31 UTC (permalink / raw) To: Fedor Pchelkin Cc: Latchesar Ionkov, Eric Van Hensbergen, Christian Schoenebeck, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project Fedor Pchelkin wrote on Tue, Dec 05, 2023 at 12:19:50PM +0300: > If an error occurs while processing an array of strings in p9pdu_vreadf > then uninitialized members of *wnames array are freed. > > Fix this by iterating over only lower indices of the array. Also handle > possible uninit *wnames usage if first p9pdu_readf() call inside 'T' case > fails. > > 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. While I agree it's good to initialize it in general, how is that a problem here? Do we have users that'd ignore the return code and try to use *wnames? (The first initialization is required in case the first p9pdu_readf fails and *wnames had a non-null initial value, but the second is unrelated) I don't mind the change even if there isn't but let's add a word in the commit message. > As an answer to Dominique's comment: my organization marks this > statement in all commits. Fair enough, I think you'd get more internet points with a 'Reported-by' but I see plenty of such messages in old commits and this isn't something I want to argue about -- ok. -- Dominique Martinet | Asmadeus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2023-12-05 9:31 ` Dominique Martinet @ 2023-12-05 12:15 ` Fedor Pchelkin 2023-12-05 12:43 ` Dominique Martinet 0 siblings, 1 reply; 18+ messages in thread From: Fedor Pchelkin @ 2023-12-05 12:15 UTC (permalink / raw) To: Dominique Martinet Cc: Latchesar Ionkov, Eric Van Hensbergen, Christian Schoenebeck, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project On 23/12/05 06:31PM, Dominique Martinet wrote: > Fedor Pchelkin wrote on Tue, Dec 05, 2023 at 12:19:50PM +0300: > > If an error occurs while processing an array of strings in p9pdu_vreadf > > then uninitialized members of *wnames array are freed. > > > > Fix this by iterating over only lower indices of the array. Also handle > > possible uninit *wnames usage if first p9pdu_readf() call inside 'T' case > > fails. > > > > 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. > > While I agree it's good to initialize it in general, how is that a > problem here? Do we have users that'd ignore the return code and try to > use *wnames? > (The first initialization is required in case the first p9pdu_readf > fails and *wnames had a non-null initial value, but the second is > unrelated) > My initial concern was just about the statement you wrote in parenthesis. Case 'T' can be provided with non-null initial *wnames value, and if the first p9pdu_readf() call there fails then *wnames is invalidly freed in error handling path here: case 'T':{ [...] if (errcode) { if (*wnames) { int i; for (i = 0; i < *nwname; i++) kfree((*wnames)[i]); } kfree(*wnames); *wnames = NULL; } So the first initialization is required to prevent the described error. As for the second initialization (the one located after kfree(*wnames) in error handling path - it was there all the time), I think it's better not to touch it. I've just moved kfree and null-assignment under 'if (*wnames)' statement. The concern you mentioned is about any user that'd ignore the return code and try to use *wnames (so that the second initialization makes some sense). I can't see if there is any such user but, as said before, it's better not to touch that code. > I don't mind the change even if there isn't but let's add a word in the > commit message. > OK, will do in v3. > > As an answer to Dominique's comment: my organization marks this > > statement in all commits. > > Fair enough, I think you'd get more internet points with a 'Reported-by' > but I see plenty of such messages in old commits and this isn't > something I want to argue about -- ok. > > -- > Dominique Martinet | Asmadeus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2023-12-05 12:15 ` Fedor Pchelkin @ 2023-12-05 12:43 ` Dominique Martinet 0 siblings, 0 replies; 18+ messages in thread From: Dominique Martinet @ 2023-12-05 12:43 UTC (permalink / raw) To: Fedor Pchelkin Cc: Latchesar Ionkov, Eric Van Hensbergen, Christian Schoenebeck, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project Fedor Pchelkin wrote on Tue, Dec 05, 2023 at 03:15:43PM +0300: > As for the second initialization (the one located after kfree(*wnames) in > error handling path - it was there all the time), I think it's better not > to touch it. I've just moved kfree and null-assignment under > 'if (*wnames)' statement. Ah, I somehow missed this was just moved; that doesn't change anything but doesn't hurt either, sure. > The concern you mentioned is about any user that'd ignore the return code > and try to use *wnames (so that the second initialization makes some > sense). I can't see if there is any such user but, as said before, it's > better not to touch that code. Yes, it was here before, let's leave it in. > > I don't mind the change even if there isn't but let's add a word in the > > commit message. > > OK, will do in v3. I've queued to -next as is (with the i initialized as Christian pointed out), will update if you send a new one later. Thanks, -- Dominique Martinet | Asmadeus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2023-12-05 9:19 ` [PATCH v2] " Fedor Pchelkin 2023-12-05 9:31 ` Dominique Martinet @ 2023-12-05 12:29 ` Christian Schoenebeck 2023-12-05 13:09 ` Fedor Pchelkin 1 sibling, 1 reply; 18+ messages in thread From: Christian Schoenebeck @ 2023-12-05 12:29 UTC (permalink / raw) To: Dominique Martinet, Fedor Pchelkin Cc: Fedor Pchelkin, Latchesar Ionkov, Eric Van Hensbergen, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project On Tuesday, December 5, 2023 10:19:50 AM CET Fedor Pchelkin wrote: > If an error occurs while processing an array of strings in p9pdu_vreadf > then uninitialized members of *wnames array are freed. > > Fix this by iterating over only lower indices of the array. Also handle > possible uninit *wnames usage if first p9pdu_readf() call inside 'T' case > fails. > > 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. > > net/9p/protocol.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/net/9p/protocol.c b/net/9p/protocol.c > index 4e3a2a1ffcb3..043b621f8b84 100644 > --- a/net/9p/protocol.c > +++ b/net/9p/protocol.c > @@ -393,6 +393,8 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > case 'T':{ > uint16_t *nwname = va_arg(ap, uint16_t *); > char ***wnames = va_arg(ap, char ***); > + int i; > + *wnames = NULL; Consider also initializing `int i = 0;` here. Because ... > > errcode = p9pdu_readf(pdu, proto_version, > "w", nwname); > @@ -406,8 +408,6 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > } > > if (!errcode) { > - int i; > - > for (i = 0; i < *nwname; i++) { ... this block that initializes `i` is conditional. I mean it does work right now as-is, because ... > errcode = > p9pdu_readf(pdu, > @@ -421,13 +421,11 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > > if (errcode) { > if (*wnames) { > - int i; > - > - for (i = 0; i < *nwname; i++) > + while (--i >= 0) > kfree((*wnames)[i]); > + kfree(*wnames); > + *wnames = NULL; > } ... this is wrapped into `if (*wnames) {` and you initialized *wnames with NULL, but it just feels like a potential future trap somehow. Anyway, at least it looks like correct behaviour (ATM), so: Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com> > - kfree(*wnames); > - *wnames = NULL; > } > } > break; > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2023-12-05 12:29 ` Christian Schoenebeck @ 2023-12-05 13:09 ` Fedor Pchelkin 2023-12-05 18:05 ` [PATCH v3] " Fedor Pchelkin 0 siblings, 1 reply; 18+ messages in thread From: Fedor Pchelkin @ 2023-12-05 13:09 UTC (permalink / raw) To: Christian Schoenebeck Cc: Dominique Martinet, Latchesar Ionkov, Eric Van Hensbergen, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project On 23/12/05 01:29PM, Christian Schoenebeck wrote: > On Tuesday, December 5, 2023 10:19:50 AM CET Fedor Pchelkin wrote: > > If an error occurs while processing an array of strings in p9pdu_vreadf > > then uninitialized members of *wnames array are freed. > > > > Fix this by iterating over only lower indices of the array. Also handle > > possible uninit *wnames usage if first p9pdu_readf() call inside 'T' case > > fails. > > > > 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. > > > > net/9p/protocol.c | 12 +++++------- > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > diff --git a/net/9p/protocol.c b/net/9p/protocol.c > > index 4e3a2a1ffcb3..043b621f8b84 100644 > > --- a/net/9p/protocol.c > > +++ b/net/9p/protocol.c > > @@ -393,6 +393,8 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > > case 'T':{ > > uint16_t *nwname = va_arg(ap, uint16_t *); > > char ***wnames = va_arg(ap, char ***); > > + int i; > > + *wnames = NULL; > > Consider also initializing `int i = 0;` here. Because ... > The hassle with indices in this code can be eliminated with using kcalloc() instead of kmalloc_array(). It would initialize all the members to zero and later we can use the fact that kfree() is a no-op for NULL args and iterate over all the elements - this trick is ubiquitous in kernel AFAIK. But when trying to do such kind of changes, I wonder whether it would impact performance (I'm not able to test this fully) or related issues as for some reason an unsafe kmalloc_array() was originally used. If you have no objections, then I'll better prepare a new patch with this in mind. That will make the code less prone to potential errors in future. > > > > errcode = p9pdu_readf(pdu, proto_version, > > "w", nwname); > > @@ -406,8 +408,6 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > > } > > > > if (!errcode) { > > - int i; > > - > > for (i = 0; i < *nwname; i++) { > > ... this block that initializes `i` is conditional. I mean it does work right > now as-is, because ... > > > errcode = > > p9pdu_readf(pdu, > > @@ -421,13 +421,11 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > > > > if (errcode) { > > if (*wnames) { > > - int i; > > - > > - for (i = 0; i < *nwname; i++) > > + while (--i >= 0) > > kfree((*wnames)[i]); > > + kfree(*wnames); > > + *wnames = NULL; > > } > > ... this is wrapped into `if (*wnames) {` and you initialized *wnames with > NULL, but it just feels like a potential future trap somehow. > > Anyway, at least it looks like correct behaviour (ATM), so: > > Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com> > > > - kfree(*wnames); > > - *wnames = NULL; > > } > > } > > break; > > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2023-12-05 13:09 ` Fedor Pchelkin @ 2023-12-05 18:05 ` Fedor Pchelkin 2023-12-06 13:12 ` Christian Schoenebeck 0 siblings, 1 reply; 18+ messages in thread From: Fedor Pchelkin @ 2023-12-05 18:05 UTC (permalink / raw) To: Dominique Martinet Cc: Fedor Pchelkin, Christian Schoenebeck, Eric Van Hensbergen, Latchesar Ionkov, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project 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. In order not to complicate the code with array index processing, fix the problem with initializing *wnames to NULL in beginning of case 'T' and using kcalloc() to allocate and initialize the array. For assurance, nullify the failing *wnames element (the callee handles that already - e.g. see 's' case). 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. net/9p/protocol.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/net/9p/protocol.c b/net/9p/protocol.c index 4e3a2a1ffcb3..7067fb49d713 100644 --- a/net/9p/protocol.c +++ b/net/9p/protocol.c @@ -394,13 +394,14 @@ 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) { *wnames = - kmalloc_array(*nwname, - sizeof(char *), - GFP_NOFS); + kcalloc(*nwname, sizeof(char *), + GFP_NOFS); if (!*wnames) errcode = -ENOMEM; } @@ -414,8 +415,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; + } } } @@ -425,9 +428,9 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, for (i = 0; i < *nwname; i++) kfree((*wnames)[i]); + kfree(*wnames); + *wnames = NULL; } - kfree(*wnames); - *wnames = NULL; } } break; -- 2.43.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2023-12-05 18:05 ` [PATCH v3] " Fedor Pchelkin @ 2023-12-06 13:12 ` Christian Schoenebeck 2023-12-06 20:09 ` [PATCH v4] " Fedor Pchelkin 0 siblings, 1 reply; 18+ messages in thread From: Christian Schoenebeck @ 2023-12-06 13:12 UTC (permalink / raw) To: Dominique Martinet, Fedor Pchelkin Cc: Fedor Pchelkin, Eric Van Hensbergen, Latchesar Ionkov, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project On Tuesday, December 5, 2023 7:05:22 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. > > In order not to complicate the code with array index processing, fix the > problem with initializing *wnames to NULL in beginning of case 'T' and > using kcalloc() to allocate and initialize the array. For assurance, > nullify the failing *wnames element (the callee handles that already - > e.g. see 's' case). > > 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. > > net/9p/protocol.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/net/9p/protocol.c b/net/9p/protocol.c > index 4e3a2a1ffcb3..7067fb49d713 100644 > --- a/net/9p/protocol.c > +++ b/net/9p/protocol.c > @@ -394,13 +394,14 @@ 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) { > *wnames = > - kmalloc_array(*nwname, > - sizeof(char *), > - GFP_NOFS); > + kcalloc(*nwname, sizeof(char *), > + GFP_NOFS); Context of this code is transmitting directory entries, e.g. thousands of array elements. So this would always introduce performance costs. The error cases this patch addresses should happen rather rarely BTW. Another option (instead of clearing the entire array) would be just setting the last entry in the array to NULL, and the loop freeing the elements would stop at the first NULL entry. That way you don't have to worry about carrying `i` along and `i` being correctly intitalized. Would require array size +1 though. In general I agree that this code section calls out to be simplified, but I doubt that clearing the entire array is the best way to go here. > if (!*wnames) > errcode = -ENOMEM; > } > @@ -414,8 +415,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; > + } > } > } > > @@ -425,9 +428,9 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > > for (i = 0; i < *nwname; i++) > kfree((*wnames)[i]); > + kfree(*wnames); > + *wnames = NULL; > } > - kfree(*wnames); > - *wnames = NULL; > } > } > break; > ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2023-12-06 13:12 ` Christian Schoenebeck @ 2023-12-06 20:09 ` Fedor Pchelkin 2023-12-07 12:54 ` Christian Schoenebeck 2023-12-11 13:51 ` Simon Horman 0 siblings, 2 replies; 18+ messages in thread From: Fedor Pchelkin @ 2023-12-06 20:09 UTC (permalink / raw) To: Dominique Martinet, Christian Schoenebeck Cc: Fedor Pchelkin, Eric Van Hensbergen, Latchesar Ionkov, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project 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; + } } } @@ -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; -- 2.43.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v4] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2023-12-06 20:09 ` [PATCH v4] " Fedor Pchelkin @ 2023-12-07 12:54 ` Christian Schoenebeck 2023-12-11 23:21 ` Dominique Martinet 2023-12-11 13:51 ` Simon Horman 1 sibling, 1 reply; 18+ messages in thread From: Christian Schoenebeck @ 2023-12-07 12:54 UTC (permalink / raw) To: Dominique Martinet, Fedor Pchelkin Cc: Fedor Pchelkin, Eric Van Hensbergen, Latchesar Ionkov, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project 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; > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2023-12-07 12:54 ` Christian Schoenebeck @ 2023-12-11 23:21 ` Dominique Martinet 2024-01-07 7:56 ` Vitaly Chikunov 0 siblings, 1 reply; 18+ messages in thread From: Dominique Martinet @ 2023-12-11 23:21 UTC (permalink / raw) To: Christian Schoenebeck Cc: Fedor Pchelkin, Eric Van Hensbergen, Latchesar Ionkov, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project 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 Martinet | Asmadeus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2023-12-11 23:21 ` Dominique Martinet @ 2024-01-07 7:56 ` Vitaly Chikunov 2024-01-07 9:48 ` Fedor Pchelkin 2024-01-07 10:26 ` Dominique Martinet 0 siblings, 2 replies; 18+ messages in thread From: Vitaly Chikunov @ 2024-01-07 7:56 UTC (permalink / raw) To: Dominique Martinet Cc: Christian Schoenebeck, Fedor Pchelkin, Eric Van Hensbergen, Latchesar Ionkov, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Re: [PATCH v4] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2024-01-07 7:56 ` Vitaly Chikunov @ 2024-01-07 9:48 ` Fedor Pchelkin 2024-01-07 10:14 ` Vitaly Chikunov 2024-01-07 10:26 ` Dominique Martinet 1 sibling, 1 reply; 18+ messages in thread From: Fedor Pchelkin @ 2024-01-07 9:48 UTC (permalink / raw) To: Vitaly Chikunov Cc: Dominique Martinet, Christian Schoenebeck, Eric Van Hensbergen, Latchesar Ionkov, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project 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/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2024-01-07 9:48 ` Fedor Pchelkin @ 2024-01-07 10:14 ` Vitaly Chikunov 0 siblings, 0 replies; 18+ messages in thread From: Vitaly Chikunov @ 2024-01-07 10:14 UTC (permalink / raw) To: Fedor Pchelkin Cc: Dominique Martinet, Christian Schoenebeck, Eric Van Hensbergen, Latchesar Ionkov, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project 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/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2024-01-07 7:56 ` Vitaly Chikunov 2024-01-07 9:48 ` Fedor Pchelkin @ 2024-01-07 10:26 ` Dominique Martinet 1 sibling, 0 replies; 18+ messages in thread From: Dominique Martinet @ 2024-01-07 10:26 UTC (permalink / raw) To: Vitaly Chikunov Cc: Christian Schoenebeck, Fedor Pchelkin, Eric Van Hensbergen, Latchesar Ionkov, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project 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, -- Dominique Martinet | Asmadeus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4] net: 9p: avoid freeing uninit memory in p9pdu_vreadf 2023-12-06 20:09 ` [PATCH v4] " Fedor Pchelkin 2023-12-07 12:54 ` Christian Schoenebeck @ 2023-12-11 13:51 ` Simon Horman 1 sibling, 0 replies; 18+ messages in thread From: Simon Horman @ 2023-12-11 13:51 UTC (permalink / raw) To: Fedor Pchelkin Cc: Dominique Martinet, Christian Schoenebeck, Eric Van Hensbergen, Latchesar Ionkov, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, v9fs, netdev, linux-kernel, Alexey Khoroshilov, lvc-project 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> ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2024-01-07 10:26 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-12-05 8:05 [PATCH] net: 9p: avoid freeing uninit memory in p9pdu_vreadf Fedor Pchelkin 2023-12-05 9:07 ` Dominique Martinet 2023-12-05 9:19 ` [PATCH v2] " Fedor Pchelkin 2023-12-05 9:31 ` Dominique Martinet 2023-12-05 12:15 ` Fedor Pchelkin 2023-12-05 12:43 ` Dominique Martinet 2023-12-05 12:29 ` Christian Schoenebeck 2023-12-05 13:09 ` Fedor Pchelkin 2023-12-05 18:05 ` [PATCH v3] " Fedor Pchelkin 2023-12-06 13:12 ` Christian Schoenebeck 2023-12-06 20:09 ` [PATCH v4] " Fedor Pchelkin 2023-12-07 12:54 ` Christian Schoenebeck 2023-12-11 23:21 ` Dominique Martinet 2024-01-07 7:56 ` Vitaly Chikunov 2024-01-07 9:48 ` Fedor Pchelkin 2024-01-07 10:14 ` Vitaly Chikunov 2024-01-07 10:26 ` Dominique Martinet 2023-12-11 13:51 ` Simon Horman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.