| Message ID | 20230927-strncpy-drivers-misc-eeprom-idt_89hpesx-c-v1-1-08e3d45b8c05@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | bcdf91c9f9077d24153258faa4942512a340d399 |
| Headers | show |
| Series | eeprom: idt_89hpesx: replace open-coded kmemdup_nul | expand |
On Wed, Sep 27, 2023 at 05:37:06AM +0000, Justin Stitt wrote: > A malloc + strncpy + manual NUL_termination is just kmemdup_nul. Let's use > this interface as it is less error-prone and more readable. > > Also drop `csraddr_len` as it is just used in a single place and we can > just do the arithmetic in-line. > > 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> Yup, this looks correct to me. Another good case of using kmemdup_nul(). Reviewed-by: Kees Cook <keescook@chromium.org> > --- > Note: build-tested only. > --- > drivers/misc/eeprom/idt_89hpesx.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/misc/eeprom/idt_89hpesx.c b/drivers/misc/eeprom/idt_89hpesx.c > index 1d1f30b5c426..d807d08e2614 100644 > --- a/drivers/misc/eeprom/idt_89hpesx.c > +++ b/drivers/misc/eeprom/idt_89hpesx.c > @@ -905,7 +905,7 @@ static ssize_t idt_dbgfs_csr_write(struct file *filep, const char __user *ubuf, > { > struct idt_89hpesx_dev *pdev = filep->private_data; > char *colon_ch, *csraddr_str, *csrval_str; > - int ret, csraddr_len; > + int ret; > u32 csraddr, csrval; > char *buf; > > @@ -927,21 +927,16 @@ static ssize_t idt_dbgfs_csr_write(struct file *filep, const char __user *ubuf, > * no new CSR value > */ > if (colon_ch != NULL) { > - csraddr_len = colon_ch - buf; > - csraddr_str = > - kmalloc(csraddr_len + 1, GFP_KERNEL); > + /* Copy the register address to the substring buffer */ > + csraddr_str = kmemdup_nul(buf, colon_ch - buf, GFP_KERNEL); > if (csraddr_str == NULL) { > ret = -ENOMEM; > goto free_buf; > } > - /* Copy the register address to the substring buffer */ > - strncpy(csraddr_str, buf, csraddr_len); > - csraddr_str[csraddr_len] = '\0'; > /* Register value must follow the colon */ > csrval_str = colon_ch + 1; > } else /* if (str_colon == NULL) */ { > csraddr_str = (char *)buf; /* Just to shut warning up */ > - csraddr_len = strnlen(csraddr_str, count); > csrval_str = NULL; > } > > > --- > base-commit: 6465e260f48790807eef06b583b38ca9789b6072 > change-id: 20230927-strncpy-drivers-misc-eeprom-idt_89hpesx-c-b09ed5507b7d > > Best regards, > -- > Justin Stitt <justinstitt@google.com> >
diff --git a/drivers/misc/eeprom/idt_89hpesx.c b/drivers/misc/eeprom/idt_89hpesx.c index 1d1f30b5c426..d807d08e2614 100644 --- a/drivers/misc/eeprom/idt_89hpesx.c +++ b/drivers/misc/eeprom/idt_89hpesx.c @@ -905,7 +905,7 @@ static ssize_t idt_dbgfs_csr_write(struct file *filep, const char __user *ubuf, { struct idt_89hpesx_dev *pdev = filep->private_data; char *colon_ch, *csraddr_str, *csrval_str; - int ret, csraddr_len; + int ret; u32 csraddr, csrval; char *buf; @@ -927,21 +927,16 @@ static ssize_t idt_dbgfs_csr_write(struct file *filep, const char __user *ubuf, * no new CSR value */ if (colon_ch != NULL) { - csraddr_len = colon_ch - buf; - csraddr_str = - kmalloc(csraddr_len + 1, GFP_KERNEL); + /* Copy the register address to the substring buffer */ + csraddr_str = kmemdup_nul(buf, colon_ch - buf, GFP_KERNEL); if (csraddr_str == NULL) { ret = -ENOMEM; goto free_buf; } - /* Copy the register address to the substring buffer */ - strncpy(csraddr_str, buf, csraddr_len); - csraddr_str[csraddr_len] = '\0'; /* Register value must follow the colon */ csrval_str = colon_ch + 1; } else /* if (str_colon == NULL) */ { csraddr_str = (char *)buf; /* Just to shut warning up */ - csraddr_len = strnlen(csraddr_str, count); csrval_str = NULL; }
A malloc + strncpy + manual NUL_termination is just kmemdup_nul. Let's use this interface as it is less error-prone and more readable. Also drop `csraddr_len` as it is just used in a single place and we can just do the arithmetic in-line. 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> --- Note: build-tested only. --- drivers/misc/eeprom/idt_89hpesx.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) --- base-commit: 6465e260f48790807eef06b583b38ca9789b6072 change-id: 20230927-strncpy-drivers-misc-eeprom-idt_89hpesx-c-b09ed5507b7d Best regards, -- Justin Stitt <justinstitt@google.com>