Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Mon, 2 Oct 2023 10:50:48 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Justin Stitt <justinstitt@google.com>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] eeprom: idt_89hpesx: replace open-coded kmemdup_nul
Message-ID: <202310021050.B6A9651@keescook>
References: <20230927-strncpy-drivers-misc-eeprom-idt_89hpesx-c-v1-1-08e3d45b8c05@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230927-strncpy-drivers-misc-eeprom-idt_89hpesx-c-v1-1-08e3d45b8c05@google.com>
Precedence: bulk

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>
> 

-- 
Kees Cook