On Tue, Sep 12, 2023 at 5:19 PM Corey Minyard <miny...@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-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.
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 <justinst...@google.com> > >
