* [PATCH] usb: gadget: function: printer: Replace strlcpy with strscpy
@ 2023-06-13 0:43 Azeem Shaikh
2023-06-13 19:30 ` Kees Cook
0 siblings, 1 reply; 5+ messages in thread
From: Azeem Shaikh @ 2023-06-13 0:43 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-hardening, Azeem Shaikh, linux-usb, linux-kernel,
Albert Briscoe, Benjamin Tissoires, Andrzej Pietrasiewicz
strlcpy() reads the entire source buffer first.
This read may exceed the destination size limit.
This is both inefficient and can lead to linear read
overflows if a source string is not NUL-terminated [1].
In an effort to remove strlcpy() completely [2], replace
strlcpy() here with strscpy().
Direct replacement is safe here since return value of -E2BIG
is used to check for truncation instead of sizeof(dest).
[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
[2] https://github.com/KSPP/linux/issues/89
Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
---
drivers/usb/gadget/function/f_printer.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
index 28db3e336e7d..10dc7c55c25f 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -1211,8 +1211,8 @@ static ssize_t f_printer_opts_pnp_string_show(struct config_item *item,
if (!opts->pnp_string)
goto unlock;
- result = strlcpy(page, opts->pnp_string, PAGE_SIZE);
- if (result >= PAGE_SIZE) {
+ result = strscpy(page, opts->pnp_string, PAGE_SIZE);
+ if (result == -E2BIG) {
result = PAGE_SIZE;
} else if (page[result - 1] != '\n' && result + 1 < PAGE_SIZE) {
page[result++] = '\n';
--
2.41.0.162.gfafddb0af9-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] usb: gadget: function: printer: Replace strlcpy with strscpy
2023-06-13 0:43 [PATCH] usb: gadget: function: printer: Replace strlcpy with strscpy Azeem Shaikh
@ 2023-06-13 19:30 ` Kees Cook
2023-06-14 14:12 ` Azeem Shaikh
0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2023-06-13 19:30 UTC (permalink / raw)
To: Azeem Shaikh
Cc: Greg Kroah-Hartman, linux-hardening, linux-usb, linux-kernel,
Albert Briscoe, Benjamin Tissoires, Andrzej Pietrasiewicz
On Tue, Jun 13, 2023 at 12:43:41AM +0000, Azeem Shaikh wrote:
> strlcpy() reads the entire source buffer first.
> This read may exceed the destination size limit.
> This is both inefficient and can lead to linear read
> overflows if a source string is not NUL-terminated [1].
> In an effort to remove strlcpy() completely [2], replace
> strlcpy() here with strscpy().
>
> Direct replacement is safe here since return value of -E2BIG
> is used to check for truncation instead of sizeof(dest).
Perhaps say "PAGE_SIZE" here instead of sizeof(dest)?
>
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> [2] https://github.com/KSPP/linux/issues/89
>
> Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
> ---
> drivers/usb/gadget/function/f_printer.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
> index 28db3e336e7d..10dc7c55c25f 100644
> --- a/drivers/usb/gadget/function/f_printer.c
> +++ b/drivers/usb/gadget/function/f_printer.c
> @@ -1211,8 +1211,8 @@ static ssize_t f_printer_opts_pnp_string_show(struct config_item *item,
> if (!opts->pnp_string)
> goto unlock;
>
> - result = strlcpy(page, opts->pnp_string, PAGE_SIZE);
> - if (result >= PAGE_SIZE) {
> + result = strscpy(page, opts->pnp_string, PAGE_SIZE);
> + if (result == -E2BIG) {
I think "< 1" might be a better test here.
> result = PAGE_SIZE;
> } else if (page[result - 1] != '\n' && result + 1 < PAGE_SIZE) {
> page[result++] = '\n';
> --
> 2.41.0.162.gfafddb0af9-goog
>
>
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] usb: gadget: function: printer: Replace strlcpy with strscpy
2023-06-13 19:30 ` Kees Cook
@ 2023-06-14 14:12 ` Azeem Shaikh
2023-06-14 17:51 ` Kees Cook
0 siblings, 1 reply; 5+ messages in thread
From: Azeem Shaikh @ 2023-06-14 14:12 UTC (permalink / raw)
To: Kees Cook
Cc: Greg Kroah-Hartman, linux-hardening, linux-usb, linux-kernel,
Albert Briscoe, Benjamin Tissoires, Andrzej Pietrasiewicz
On Tue, Jun 13, 2023 at 3:30 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Jun 13, 2023 at 12:43:41AM +0000, Azeem Shaikh wrote:
> > strlcpy() reads the entire source buffer first.
> > This read may exceed the destination size limit.
> > This is both inefficient and can lead to linear read
> > overflows if a source string is not NUL-terminated [1].
> > In an effort to remove strlcpy() completely [2], replace
> > strlcpy() here with strscpy().
> >
> > Direct replacement is safe here since return value of -E2BIG
> > is used to check for truncation instead of sizeof(dest).
>
> Perhaps say "PAGE_SIZE" here instead of sizeof(dest)?
>
Ack.
> >
> > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> > [2] https://github.com/KSPP/linux/issues/89
> >
> > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
> > ---
> > drivers/usb/gadget/function/f_printer.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
> > index 28db3e336e7d..10dc7c55c25f 100644
> > --- a/drivers/usb/gadget/function/f_printer.c
> > +++ b/drivers/usb/gadget/function/f_printer.c
> > @@ -1211,8 +1211,8 @@ static ssize_t f_printer_opts_pnp_string_show(struct config_item *item,
> > if (!opts->pnp_string)
> > goto unlock;
> >
> > - result = strlcpy(page, opts->pnp_string, PAGE_SIZE);
> > - if (result >= PAGE_SIZE) {
> > + result = strscpy(page, opts->pnp_string, PAGE_SIZE);
> > + if (result == -E2BIG) {
>
> I think "< 1" might be a better test here.
>
Curious, why "< 1" instead of "< 0"?
> > result = PAGE_SIZE;
> > } else if (page[result - 1] != '\n' && result + 1 < PAGE_SIZE) {
> > page[result++] = '\n';
> > --
> > 2.41.0.162.gfafddb0af9-goog
> >
> >
>
> -Kees
>
> --
> Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] usb: gadget: function: printer: Replace strlcpy with strscpy
2023-06-14 14:12 ` Azeem Shaikh
@ 2023-06-14 17:51 ` Kees Cook
2023-06-15 18:09 ` Azeem Shaikh
0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2023-06-14 17:51 UTC (permalink / raw)
To: Azeem Shaikh
Cc: Greg Kroah-Hartman, linux-hardening, linux-usb, linux-kernel,
Albert Briscoe, Benjamin Tissoires, Andrzej Pietrasiewicz
On Wed, Jun 14, 2023 at 10:12:14AM -0400, Azeem Shaikh wrote:
> On Tue, Jun 13, 2023 at 3:30 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Tue, Jun 13, 2023 at 12:43:41AM +0000, Azeem Shaikh wrote:
> [...]
> > > - result = strlcpy(page, opts->pnp_string, PAGE_SIZE);
> > > - if (result >= PAGE_SIZE) {
> > > + result = strscpy(page, opts->pnp_string, PAGE_SIZE);
> > > + if (result == -E2BIG) {
> >
> > I think "< 1" might be a better test here.
>
> Curious, why "< 1" instead of "< 0"?
>
> > > result = PAGE_SIZE;
> > > } else if (page[result - 1] != '\n' && result + 1 < PAGE_SIZE) {
It's for this case above where "result" may be used in an array index,
and if it's 0 or less, there will be a negative array index (due to the "-
1"). So, here, it needs to be "< 1" instead of the more traditional "< 0".
--
Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] usb: gadget: function: printer: Replace strlcpy with strscpy
2023-06-14 17:51 ` Kees Cook
@ 2023-06-15 18:09 ` Azeem Shaikh
0 siblings, 0 replies; 5+ messages in thread
From: Azeem Shaikh @ 2023-06-15 18:09 UTC (permalink / raw)
To: Kees Cook
Cc: Greg Kroah-Hartman, linux-hardening, linux-usb, linux-kernel,
Albert Briscoe, Benjamin Tissoires, Andrzej Pietrasiewicz
On Wed, Jun 14, 2023 at 1:51 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Jun 14, 2023 at 10:12:14AM -0400, Azeem Shaikh wrote:
> > On Tue, Jun 13, 2023 at 3:30 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Tue, Jun 13, 2023 at 12:43:41AM +0000, Azeem Shaikh wrote:
> > [...]
> > > > - result = strlcpy(page, opts->pnp_string, PAGE_SIZE);
> > > > - if (result >= PAGE_SIZE) {
> > > > + result = strscpy(page, opts->pnp_string, PAGE_SIZE);
> > > > + if (result == -E2BIG) {
> > >
> > > I think "< 1" might be a better test here.
> >
> > Curious, why "< 1" instead of "< 0"?
> >
> > > > result = PAGE_SIZE;
> > > > } else if (page[result - 1] != '\n' && result + 1 < PAGE_SIZE) {
>
> It's for this case above where "result" may be used in an array index,
> and if it's 0 or less, there will be a negative array index (due to the "-
> 1"). So, here, it needs to be "< 1" instead of the more traditional "< 0".
>
Makes sense. Sent out a v2.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-06-15 18:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-13 0:43 [PATCH] usb: gadget: function: printer: Replace strlcpy with strscpy Azeem Shaikh
2023-06-13 19:30 ` Kees Cook
2023-06-14 14:12 ` Azeem Shaikh
2023-06-14 17:51 ` Kees Cook
2023-06-15 18:09 ` Azeem Shaikh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox