| Message ID | 20230630110401.2360746-1-yguoaz@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: configfs: Prevent buffer overrun in usb_string_copy | expand |
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
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
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
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
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;
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(-)