| Message ID | 20231205091952.24754-1-pchelkin@ispras.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] net: 9p: avoid freeing uninit memory in p9pdu_vreadf | expand |
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.
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
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; >
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,
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; > > > >
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;
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(-)