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-harden...@vger.kernel.org > Cc: Kees Cook <keesc...@chromium.org> > Signed-off-by: Justin Stitt <justinst...@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 <justinst...@google.com> >
