2023-09-27 10:20:54

by Justin Stitt

Subject: [PATCH] eeprom: idt_89hpesx: replace open-coded kmemdup_nul

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: [email protected]
Cc: Kees Cook <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
---
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 <[email protected]>

2023-10-02 21:18:45

by Kees Cook

Subject: Re: [PATCH] eeprom: idt_89hpesx: replace open-coded kmemdup_nul

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: [email protected]
> Cc: Kees Cook <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>

Yup, this looks correct to me. Another good case of using kmemdup_nul().

Reviewed-by: Kees Cook <[email protected]>

> ---
> 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 <[email protected]>
>

--
Kees Cook