linux-kernel - Re: [PATCH] habanalabs/gaudi: refactor deprecated strncpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202308251321.E19D664F0@keescook>
Date:   Fri, 25 Aug 2023 13:31:52 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Justin Stitt <justinstitt@...gle.com>
Cc:     Oded Gabbay <ogabbay@...nel.org>, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] habanalabs/gaudi: refactor deprecated strncpy

On Thu, Aug 24, 2023 at 08:41:26PM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> 
> A suitable replacement is `strscpy` [2] due to the fact that it
> guarantees NUL-termination on its destination buffer argument which is
> _not_ the case for `strncpy`!

For strncpy replacements the commit log needs to always address 2 items,
and really get spelled out for reviewers since the diff context is
rarely enough information to judge the safety of the change:

1) How did you determine that the destination buffer does or does not
   require %NUL termination?

2) How did you determine that the destination buffer does or does not
   need to be %NUL padded?

> 
> Link: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@...r.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@...gle.com>
> ---
> Note: build-tested only
> ---
>  drivers/accel/habanalabs/gaudi/gaudi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/accel/habanalabs/gaudi/gaudi.c b/drivers/accel/habanalabs/gaudi/gaudi.c
> index 056e2ef44afb..f175456cdef0 100644
> --- a/drivers/accel/habanalabs/gaudi/gaudi.c
> +++ b/drivers/accel/habanalabs/gaudi/gaudi.c
> @@ -660,7 +660,7 @@ static int gaudi_set_fixed_properties(struct hl_device *hdev)
>  	prop->pcie_dbi_base_address = mmPCIE_DBI_BASE;
>  	prop->pcie_aux_dbi_reg_addr = CFG_BASE + mmPCIE_AUX_DBI;
>  
> -	strncpy(prop->cpucp_info.card_name, GAUDI_DEFAULT_CARD_NAME,
> +	strscpy(prop->cpucp_info.card_name, GAUDI_DEFAULT_CARD_NAME,
>  					CARD_NAME_MAX_LEN);
>  
>  	prop->max_pending_cs = GAUDI_MAX_PENDING_CS;
> @@ -8000,7 +8000,7 @@ static int gaudi_cpucp_info_get(struct hl_device *hdev)
>  		return rc;
>  
>  	if (!strlen(prop->cpucp_info.card_name))
> -		strncpy(prop->cpucp_info.card_name, GAUDI_DEFAULT_CARD_NAME,
> +		strscpy(prop->cpucp_info.card_name, GAUDI_DEFAULT_CARD_NAME,
>  				CARD_NAME_MAX_LEN);

When I look at this string, I see it gets copied out to userspace
directly:

static int hw_ip_info(struct hl_device *hdev, struct hl_info_args *args)
{
        struct hl_info_hw_ip_info hw_ip = {0};
	...
        memcpy(hw_ip.card_name, prop->cpucp_info.card_name,
                min(CARD_NAME_MAX_LEN, HL_INFO_CARD_NAME_MAX_LEN));
	...
        return copy_to_user(out, &hw_ip,
                min((size_t) size, sizeof(hw_ip))) ? -EFAULT : 0;
}

So if "prop" isn't zero initialized, this is now a kernel memory content
exposure, due to the lack of padding.

If I track the allocation of "hdev" all the way back, I can see it is,
however, zero initialized:

static int create_hdev(struct hl_device **dev, struct pci_dev *pdev)
...
        hdev = kzalloc(sizeof(*hdev), GFP_KERNEL);


But since it's still sent via copy_to_user(), the more defensive
replacement here should be strscpy_pad().

(Also, I think you can do all three files in the same patch -- it's
operating on the same struct and in the same way.)

-Kees

-- 
Kees Cook