All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ipmi: refactor deprecated strncpy
@ 2023-09-12 23:43 Justin Stitt
  2023-09-13  0:19 ` Corey Minyard
  0 siblings, 1 reply; 5+ messages in thread
From: Justin Stitt @ 2023-09-12 23:43 UTC (permalink / raw)
  To: Corey Minyard
  Cc: openipmi-developer, linux-kernel, linux-hardening, Kees Cook,
	Justin Stitt

`strncpy` is deprecated for use on NUL-terminated destination strings [1].

In this case, strncpy is being used specifically for its NUL-padding
behavior (and has been commented as such). We can use a more robust and
less ambiguous interface in `strscpy_pad` which makes the code more
readable and even eliminates the need for that comment.

Let's also use `strnlen` instead of `strlen()` with an upper-bounds
check as this is intrinsically a part of `strnlen`.

Also included in this patch is a simple 1:1 change of `strncpy` to
`strscpy` for ipmi_ssif.c. If NUL-padding is wanted here as well then we
should opt again for `strscpy_pad`.

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 drivers/char/ipmi/ipmi_msghandler.c | 11 +++--------
 drivers/char/ipmi/ipmi_ssif.c       |  2 +-
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 186f1fee7534..04f7622cb703 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -5377,20 +5377,15 @@ static void send_panic_events(struct ipmi_smi *intf, char *str)
 
 	j = 0;
 	while (*p) {
-		int size = strlen(p);
+		int size = strnlen(p, 11);
 
-		if (size > 11)
-			size = 11;
 		data[0] = 0;
 		data[1] = 0;
 		data[2] = 0xf0; /* OEM event without timestamp. */
 		data[3] = intf->addrinfo[0].address;
 		data[4] = j++; /* sequence # */
-		/*
-		 * Always give 11 bytes, so strncpy will fill
-		 * it with zeroes for me.
-		 */
-		strncpy(data+5, p, 11);
+
+		strscpy_pad(data+5, p, 11);
 		p += size;
 
 		ipmi_panic_request_and_wait(intf, &addr, &msg);
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 3b921c78ba08..edcb83765dce 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -1940,7 +1940,7 @@ static int new_ssif_client(int addr, char *adapter_name,
 		}
 	}
 
-	strncpy(addr_info->binfo.type, DEVICE_NAME,
+	strscpy(addr_info->binfo.type, DEVICE_NAME,
 		sizeof(addr_info->binfo.type));
 	addr_info->binfo.addr = addr;
 	addr_info->binfo.platform_data = addr_info;

---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd

Best regards,
--
Justin Stitt <justinstitt@google.com>


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] ipmi: refactor deprecated strncpy
  2023-09-12 23:43 [PATCH] ipmi: refactor deprecated strncpy Justin Stitt
@ 2023-09-13  0:19 ` Corey Minyard
  2023-09-13  0:55   ` Justin Stitt
  0 siblings, 1 reply; 5+ messages in thread
From: Corey Minyard @ 2023-09-13  0:19 UTC (permalink / raw)
  To: Justin Stitt; +Cc: openipmi-developer, linux-kernel, linux-hardening, Kees Cook

On Tue, Sep 12, 2023 at 11:43:05PM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> 
> In this case, strncpy is being used specifically for its NUL-padding
> behavior (and has been commented as such). We can use a more robust and
> less ambiguous interface in `strscpy_pad` which makes the code more
> readable and even eliminates the need for that comment.
> 
> Let's also use `strnlen` instead of `strlen()` with an upper-bounds
> check as this is intrinsically a part of `strnlen`.
> 
> Also included in this patch is a simple 1:1 change of `strncpy` to
> `strscpy` for ipmi_ssif.c. If NUL-padding is wanted here as well then we
> should opt again for `strscpy_pad`.
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
>  drivers/char/ipmi/ipmi_msghandler.c | 11 +++--------
>  drivers/char/ipmi/ipmi_ssif.c       |  2 +-
>  2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index 186f1fee7534..04f7622cb703 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -5377,20 +5377,15 @@ static void send_panic_events(struct ipmi_smi *intf, char *str)
>  
>  	j = 0;
>  	while (*p) {
> -		int size = strlen(p);
> +		int size = strnlen(p, 11);
>  
> -		if (size > 11)
> -			size = 11;
>  		data[0] = 0;
>  		data[1] = 0;
>  		data[2] = 0xf0; /* OEM event without timestamp. */
>  		data[3] = intf->addrinfo[0].address;
>  		data[4] = j++; /* sequence # */
> -		/*
> -		 * Always give 11 bytes, so strncpy will fill
> -		 * it with zeroes for me.
> -		 */
> -		strncpy(data+5, p, 11);
> +
> +		strscpy_pad(data+5, p, 11);

This is incorrect, the destination should *not* be nil terminated if the
destination is full.  strncpy does exactly what is needed here.

A comment should be added here, this is not the first time this has been
brought up.

>  		p += size;
>  
>  		ipmi_panic_request_and_wait(intf, &addr, &msg);
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 3b921c78ba08..edcb83765dce 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -1940,7 +1940,7 @@ static int new_ssif_client(int addr, char *adapter_name,
>  		}
>  	}
>  
> -	strncpy(addr_info->binfo.type, DEVICE_NAME,
> +	strscpy(addr_info->binfo.type, DEVICE_NAME,
>  		sizeof(addr_info->binfo.type));

This one is good.

-corey

>  	addr_info->binfo.addr = addr;
>  	addr_info->binfo.platform_data = addr_info;
> 
> ---
> base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd
> 
> Best regards,
> --
> Justin Stitt <justinstitt@google.com>
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] ipmi: refactor deprecated strncpy
  2023-09-13  0:19 ` Corey Minyard
@ 2023-09-13  0:55   ` Justin Stitt
  2023-09-13  0:58     ` Justin Stitt
  2023-09-13 12:12     ` Corey Minyard
  0 siblings, 2 replies; 5+ messages in thread
From: Justin Stitt @ 2023-09-13  0:55 UTC (permalink / raw)
  To: minyard; +Cc: openipmi-developer, linux-kernel, linux-hardening, Kees Cook

On Tue, Sep 12, 2023 at 5:19 PM Corey Minyard <minyard@acm.org> wrote:
>
> On Tue, Sep 12, 2023 at 11:43:05PM +0000, Justin Stitt wrote:
> > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> >
> > In this case, strncpy is being used specifically for its NUL-padding
> > behavior (and has been commented as such). We can use a more robust and
> > less ambiguous interface in `strscpy_pad` which makes the code more
> > readable and even eliminates the need for that comment.
> >
> > Let's also use `strnlen` instead of `strlen()` with an upper-bounds
> > check as this is intrinsically a part of `strnlen`.
> >
> > Also included in this patch is a simple 1:1 change of `strncpy` to
> > `strscpy` for ipmi_ssif.c. If NUL-padding is wanted here as well then we
> > should opt again for `strscpy_pad`.
> >
> > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> > Link: https://github.com/KSPP/linux/issues/90
> > Cc: linux-hardening@vger.kernel.org
> > Cc: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > ---
> >  drivers/char/ipmi/ipmi_msghandler.c | 11 +++--------
> >  drivers/char/ipmi/ipmi_ssif.c       |  2 +-
> >  2 files changed, 4 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> > index 186f1fee7534..04f7622cb703 100644
> > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > @@ -5377,20 +5377,15 @@ static void send_panic_events(struct ipmi_smi *intf, char *str)
> >
> >       j = 0;
> >       while (*p) {
> > -             int size = strlen(p);
> > +             int size = strnlen(p, 11);
> >
> > -             if (size > 11)
> > -                     size = 11;
> >               data[0] = 0;
> >               data[1] = 0;
> >               data[2] = 0xf0; /* OEM event without timestamp. */
> >               data[3] = intf->addrinfo[0].address;
> >               data[4] = j++; /* sequence # */
> > -             /*
> > -              * Always give 11 bytes, so strncpy will fill
> > -              * it with zeroes for me.
> > -              */
> > -             strncpy(data+5, p, 11);
> > +
> > +             strscpy_pad(data+5, p, 11);
>
> This is incorrect, the destination should *not* be nil terminated if the
> destination is full.  strncpy does exactly what is needed here.

Could we use `memcpy_and_pad()` as this matches the behavior of
strncpy in this case? I understand strncpy works here but I'm really
keen on snuffing out all its uses -- treewide.

>
> A comment should be added here, this is not the first time this has been
> brought up.
>
> >               p += size;
> >
> >               ipmi_panic_request_and_wait(intf, &addr, &msg);
> > diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> > index 3b921c78ba08..edcb83765dce 100644
> > --- a/drivers/char/ipmi/ipmi_ssif.c
> > +++ b/drivers/char/ipmi/ipmi_ssif.c
> > @@ -1940,7 +1940,7 @@ static int new_ssif_client(int addr, char *adapter_name,
> >               }
> >       }
> >
> > -     strncpy(addr_info->binfo.type, DEVICE_NAME,
> > +     strscpy(addr_info->binfo.type, DEVICE_NAME,
> >               sizeof(addr_info->binfo.type));
>
> This one is good.
>
> -corey
>
> >       addr_info->binfo.addr = addr;
> >       addr_info->binfo.platform_data = addr_info;
> >
> > ---
> > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> > change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd
> >
> > Best regards,
> > --
> > Justin Stitt <justinstitt@google.com>
> >

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] ipmi: refactor deprecated strncpy
  2023-09-13  0:55   ` Justin Stitt
@ 2023-09-13  0:58     ` Justin Stitt
  2023-09-13 12:12     ` Corey Minyard
  1 sibling, 0 replies; 5+ messages in thread
From: Justin Stitt @ 2023-09-13  0:58 UTC (permalink / raw)
  To: minyard; +Cc: openipmi-developer, linux-kernel, linux-hardening, Kees Cook

On Tue, Sep 12, 2023 at 5:55 PM Justin Stitt <justinstitt@google.com> wrote:
>
> On Tue, Sep 12, 2023 at 5:19 PM Corey Minyard <minyard@acm.org> wrote:
> >
> > On Tue, Sep 12, 2023 at 11:43:05PM +0000, Justin Stitt wrote:
> > > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> > >
> > > In this case, strncpy is being used specifically for its NUL-padding
> > > behavior (and has been commented as such). We can use a more robust and
> > > less ambiguous interface in `strscpy_pad` which makes the code more
> > > readable and even eliminates the need for that comment.
> > >
> > > Let's also use `strnlen` instead of `strlen()` with an upper-bounds
> > > check as this is intrinsically a part of `strnlen`.
> > >
> > > Also included in this patch is a simple 1:1 change of `strncpy` to
> > > `strscpy` for ipmi_ssif.c. If NUL-padding is wanted here as well then we
> > > should opt again for `strscpy_pad`.
> > >
> > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> > > Link: https://github.com/KSPP/linux/issues/90
> > > Cc: linux-hardening@vger.kernel.org
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > > ---
> > >  drivers/char/ipmi/ipmi_msghandler.c | 11 +++--------
> > >  drivers/char/ipmi/ipmi_ssif.c       |  2 +-
> > >  2 files changed, 4 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> > > index 186f1fee7534..04f7622cb703 100644
> > > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > > @@ -5377,20 +5377,15 @@ static void send_panic_events(struct ipmi_smi *intf, char *str)
> > >
> > >       j = 0;
> > >       while (*p) {
> > > -             int size = strlen(p);
> > > +             int size = strnlen(p, 11);
> > >
> > > -             if (size > 11)
> > > -                     size = 11;
> > >               data[0] = 0;
> > >               data[1] = 0;
> > >               data[2] = 0xf0; /* OEM event without timestamp. */
> > >               data[3] = intf->addrinfo[0].address;
> > >               data[4] = j++; /* sequence # */
> > > -             /*
> > > -              * Always give 11 bytes, so strncpy will fill
> > > -              * it with zeroes for me.
> > > -              */
> > > -             strncpy(data+5, p, 11);
> > > +
> > > +             strscpy_pad(data+5, p, 11);
> >
> > This is incorrect, the destination should *not* be nil terminated if the
> > destination is full.  strncpy does exactly what is needed here.
>
> Could we use `memcpy_and_pad()` as this matches the behavior of
> strncpy in this case? I understand strncpy works here but I'm really
> keen on snuffing out all its uses -- treewide.

^ I mean something like the following:
|    memcpy_and_pad(data+5, 11, p, size, '\0');

as this is explicit in its behavior.

>
> >
> > A comment should be added here, this is not the first time this has been
> > brought up.
> >
> > >               p += size;
> > >
> > >               ipmi_panic_request_and_wait(intf, &addr, &msg);
> > > diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> > > index 3b921c78ba08..edcb83765dce 100644
> > > --- a/drivers/char/ipmi/ipmi_ssif.c
> > > +++ b/drivers/char/ipmi/ipmi_ssif.c
> > > @@ -1940,7 +1940,7 @@ static int new_ssif_client(int addr, char *adapter_name,
> > >               }
> > >       }
> > >
> > > -     strncpy(addr_info->binfo.type, DEVICE_NAME,
> > > +     strscpy(addr_info->binfo.type, DEVICE_NAME,
> > >               sizeof(addr_info->binfo.type));
> >
> > This one is good.
> >
> > -corey
> >
> > >       addr_info->binfo.addr = addr;
> > >       addr_info->binfo.platform_data = addr_info;
> > >
> > > ---
> > > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> > > change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd
> > >
> > > Best regards,
> > > --
> > > Justin Stitt <justinstitt@google.com>
> > >

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] ipmi: refactor deprecated strncpy
  2023-09-13  0:55   ` Justin Stitt
  2023-09-13  0:58     ` Justin Stitt
@ 2023-09-13 12:12     ` Corey Minyard
  1 sibling, 0 replies; 5+ messages in thread
From: Corey Minyard @ 2023-09-13 12:12 UTC (permalink / raw)
  To: Justin Stitt; +Cc: openipmi-developer, linux-kernel, linux-hardening, Kees Cook

On Tue, Sep 12, 2023 at 05:55:02PM -0700, Justin Stitt wrote:
> On Tue, Sep 12, 2023 at 5:19 PM Corey Minyard <minyard@acm.org> wrote:
> >
> > On Tue, Sep 12, 2023 at 11:43:05PM +0000, Justin Stitt wrote:
> > > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> > >
> > > In this case, strncpy is being used specifically for its NUL-padding
> > > behavior (and has been commented as such). We can use a more robust and
> > > less ambiguous interface in `strscpy_pad` which makes the code more
> > > readable and even eliminates the need for that comment.
> > >
> > > Let's also use `strnlen` instead of `strlen()` with an upper-bounds
> > > check as this is intrinsically a part of `strnlen`.
> > >
> > > Also included in this patch is a simple 1:1 change of `strncpy` to
> > > `strscpy` for ipmi_ssif.c. If NUL-padding is wanted here as well then we
> > > should opt again for `strscpy_pad`.
> > >
> > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> > > Link: https://github.com/KSPP/linux/issues/90
> > > Cc: linux-hardening@vger.kernel.org
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > > ---
> > >  drivers/char/ipmi/ipmi_msghandler.c | 11 +++--------
> > >  drivers/char/ipmi/ipmi_ssif.c       |  2 +-
> > >  2 files changed, 4 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> > > index 186f1fee7534..04f7622cb703 100644
> > > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > > @@ -5377,20 +5377,15 @@ static void send_panic_events(struct ipmi_smi *intf, char *str)
> > >
> > >       j = 0;
> > >       while (*p) {
> > > -             int size = strlen(p);
> > > +             int size = strnlen(p, 11);
> > >
> > > -             if (size > 11)
> > > -                     size = 11;
> > >               data[0] = 0;
> > >               data[1] = 0;
> > >               data[2] = 0xf0; /* OEM event without timestamp. */
> > >               data[3] = intf->addrinfo[0].address;
> > >               data[4] = j++; /* sequence # */
> > > -             /*
> > > -              * Always give 11 bytes, so strncpy will fill
> > > -              * it with zeroes for me.
> > > -              */
> > > -             strncpy(data+5, p, 11);
> > > +
> > > +             strscpy_pad(data+5, p, 11);
> >
> > This is incorrect, the destination should *not* be nil terminated if the
> > destination is full.  strncpy does exactly what is needed here.
> 
> Could we use `memcpy_and_pad()` as this matches the behavior of
> strncpy in this case? I understand strncpy works here but I'm really
> keen on snuffing out all its uses -- treewide.

Sure, I think "memcpy_and_pad(data + 5, 11, p, size, 0);" should work.
And that's self-documenting.

-corey

> 
> >
> > A comment should be added here, this is not the first time this has been
> > brought up.
> >
> > >               p += size;
> > >
> > >               ipmi_panic_request_and_wait(intf, &addr, &msg);
> > > diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> > > index 3b921c78ba08..edcb83765dce 100644
> > > --- a/drivers/char/ipmi/ipmi_ssif.c
> > > +++ b/drivers/char/ipmi/ipmi_ssif.c
> > > @@ -1940,7 +1940,7 @@ static int new_ssif_client(int addr, char *adapter_name,
> > >               }
> > >       }
> > >
> > > -     strncpy(addr_info->binfo.type, DEVICE_NAME,
> > > +     strscpy(addr_info->binfo.type, DEVICE_NAME,
> > >               sizeof(addr_info->binfo.type));
> >
> > This one is good.
> >
> > -corey
> >
> > >       addr_info->binfo.addr = addr;
> > >       addr_info->binfo.platform_data = addr_info;
> > >
> > > ---
> > > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> > > change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd
> > >
> > > Best regards,
> > > --
> > > Justin Stitt <justinstitt@google.com>
> > >

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-09-13 12:12 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-12 23:43 [PATCH] ipmi: refactor deprecated strncpy Justin Stitt
2023-09-13  0:19 ` Corey Minyard
2023-09-13  0:55   ` Justin Stitt
2023-09-13  0:58     ` Justin Stitt
2023-09-13 12:12     ` Corey Minyard

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.