[PATCH] usb: gadget: configfs: Prevent buffer overrun in usb_string_copy

[PATCH] usb: gadget: configfs: Prevent buffer overrun in usb_string_copy

[Yiyuan Guo]

In usb_string_copy(), when `strlen(s) == 0`, `str[ret - 1]` accesses at
index -1. Add a check to prevent buffer overrun when `s` is empty.

Signed-off-by: Yiyuan Guo <yguoaz@gmail.com>
---
 drivers/usb/gadget/configfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 4c639e9ddedc..457dbc267964 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -127,7 +127,7 @@ static int usb_string_copy(const char *s, char **s_copy)
 			return -ENOMEM;
 	}
 	strcpy(str, s);
-	if (str[ret - 1] == '\n')
+	if (ret && str[ret - 1] == '\n')
 		str[ret - 1] = '\0';
 	*s_copy = str;
 	return 0;
-- 
2.25.1

Re: [PATCH] usb: gadget: configfs: Prevent buffer overrun in usb_string_copy

[Greg KH]

On Fri, Jun 30, 2023 at 07:04:01PM +0800, Yiyuan Guo wrote:
> In usb_string_copy(), when `strlen(s) == 0`, `str[ret - 1]` accesses at
> index -1. Add a check to prevent buffer overrun when `s` is empty.

It's an underrun, right?

And how can strlen(s) ever be 0 here?

How did you test this and how did you trigger it?

And what commit id does this fix?

And how was this found?

thanks,

greg k-h

Re: [PATCH] usb: gadget: configfs: Prevent buffer overrun in usb_string_copy

[yguoaz]

This is an underrun issue found by a static analysis tool (under
research). I suggest the patch because the code of usb_string_copy()
rejects strings with length greater than USB_MAX_STRING_LEN,
indicating a possibility for the input string `s` to contain unwanted
data (e.g., being empty). For the empty string case, the proposed
patch simply copies '\0' in `strcpy(str, s)` without touching index -1
of `str`.

Whether `strlen(s)` could ever be zero in reality is up to the
maintainer's judgement, since I have not worked with the subsystem. So
please ignore the patch if it is ensured that `s` must be non-empty.

Thanks,
Yiyuan

On Fri, Jun 30, 2023 at 8:17 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Jun 30, 2023 at 07:04:01PM +0800, Yiyuan Guo wrote:
> > In usb_string_copy(), when `strlen(s) == 0`, `str[ret - 1]` accesses at
> > index -1. Add a check to prevent buffer overrun when `s` is empty.
>
> It's an underrun, right?
>
> And how can strlen(s) ever be 0 here?
>
> How did you test this and how did you trigger it?
>
> And what commit id does this fix?
>
> And how was this found?
>
> thanks,
>
> greg k-h

Re: [PATCH] usb: gadget: configfs: Prevent buffer overrun in usb_string_copy

[Greg KH]

On Fri, Jun 30, 2023 at 09:13:58PM +0800, yguoaz wrote:
> This is an underrun issue found by a static analysis tool (under
> research).

Then you MUST follow our research rules in order to submit patches.
Please read and follow them, otherwise we have to reject all of your
submissions.

> I suggest the patch because the code of usb_string_copy()
> rejects strings with length greater than USB_MAX_STRING_LEN,
> indicating a possibility for the input string `s` to contain unwanted
> data (e.g., being empty). For the empty string case, the proposed
> patch simply copies '\0' in `strcpy(str, s)` without touching index -1
> of `str`.
> 
> Whether `strlen(s)` could ever be zero in reality is up to the
> maintainer's judgement, since I have not worked with the subsystem. So
> please ignore the patch if it is ensured that `s` must be non-empty.

Test it and see!

good luck,

greg k-h

Re: [PATCH] usb: gadget: configfs: Prevent buffer overrun in usb_string_copy

[yguoaz]

Have done some testing. This issue cannot happen due to the protection
in `configfs_write_iter()`:

len = fill_write_buffer(buffer, from);
if (len > 0)
  len = flush_write_buffer(file, buffer, len);

Thanks for your patience,
Yiyuan

On Sat, Jul 1, 2023 at 3:48 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Jun 30, 2023 at 09:13:58PM +0800, yguoaz wrote:
> > This is an underrun issue found by a static analysis tool (under
> > research).
>
> Then you MUST follow our research rules in order to submit patches.
> Please read and follow them, otherwise we have to reject all of your
> submissions.
>
> > I suggest the patch because the code of usb_string_copy()
> > rejects strings with length greater than USB_MAX_STRING_LEN,
> > indicating a possibility for the input string `s` to contain unwanted
> > data (e.g., being empty). For the empty string case, the proposed
> > patch simply copies '\0' in `strcpy(str, s)` without touching index -1
> > of `str`.
> >
> > Whether `strlen(s)` could ever be zero in reality is up to the
> > maintainer's judgement, since I have not worked with the subsystem. So
> > please ignore the patch if it is ensured that `s` must be non-empty.
>
> Test it and see!
>
> good luck,
>
> greg k-h