| Message ID | 20230913-strncpy-drivers-char-ipmi-ipmi-v2-1-e3bc0f6e599f@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | b00839ca4cca8aa9641c121c848a553d6220ce70 |
| Headers | show |
| Series | [v2] ipmi: refactor deprecated strncpy | expand |
On Wed, Sep 13, 2023 at 05:13:04PM +0000, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. Thanks, applied to my next tree. -corey > > In this case, strncpy is being used specifically for its NUL-padding > behavior (and has been commented as such). Moreover, the destination > string is not required to be NUL-terminated [2]. > > We can use a more robust and less ambiguous interface in > `memcpy_and_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://lore.kernel.org/all/ZQEADYBl0uZ1nX60@mail.minyard.net/ [2] > 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> > --- > Changes in v2: > - use memcpy_and_pad (thanks Corey) > - Link to v1: https://lore.kernel.org/r/20230912-strncpy-drivers-char-ipmi-ipmi-v1-1-cc43e0d1cae6@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..d6f14279684d 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); > + > + memcpy_and_pad(data+5, 11, p, size, '\0'); > 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> >
diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 186f1fee7534..d6f14279684d 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); + + memcpy_and_pad(data+5, 11, p, size, '\0'); 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;
`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). Moreover, the destination string is not required to be NUL-terminated [2]. We can use a more robust and less ambiguous interface in `memcpy_and_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://lore.kernel.org/all/ZQEADYBl0uZ1nX60@mail.minyard.net/ [2] 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> --- Changes in v2: - use memcpy_and_pad (thanks Corey) - Link to v1: https://lore.kernel.org/r/20230912-strncpy-drivers-char-ipmi-ipmi-v1-1-cc43e0d1cae6@google.com --- drivers/char/ipmi/ipmi_msghandler.c | 11 +++-------- drivers/char/ipmi/ipmi_ssif.c | 2 +- 2 files changed, 4 insertions(+), 9 deletions(-) --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd Best regards, -- Justin Stitt <justinstitt@google.com>