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 <[email protected]>
---
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
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 <[email protected]>
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
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
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 <[email protected]>
> > ---
> > 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 <[email protected]>
>
> > - kfree(*wnames);
> > - *wnames = NULL;
> > }
> > }
> > break;
> >
>
>
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 <[email protected]>
> ---
> 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 <[email protected]>
> - 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.
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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;
>
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
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
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/[email protected]/
> 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/[email protected]/
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/[email protected]/
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/[email protected] )
Thanks,
--
Dominique Martinet | Asmadeus