Received-SPF: pass (google.com: domain of linux-kernel+bounces-108357-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 19 Mar 2024 19:23:43 -0700
From: Kees Cook <keescook@chromium.org>
To: Justin Stitt <justinstitt@google.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>,
	linux-scsi@vger.kernel.org, target-devel@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] scsi: target: core: replace deprecated strncpy with
 strscpy
Message-ID: <202403191908.1B6BBA3@keescook>
References: <20240318-strncpy-drivers-target-target_core_transport-c-v1-1-a086b9a0d639@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240318-strncpy-drivers-target-target_core_transport-c-v1-1-a086b9a0d639@google.com>

On Mon, Mar 18, 2024 at 10:07:50PM +0000, Justin Stitt wrote:
> strncpy() is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
> 
> We expect p_buf to be NUL-terminated based on the callsites using these
> transport_dump_* methods because they use the destination buf with C-string
> APIs like strlen() and sprintf().
> 
> 		memset(buf, 0, VPD_TMP_BUF_SIZE);			\
> 		transport_dump_vpd_ident_type(vpd, buf, VPD_TMP_BUF_SIZE); \
> 		if (len + strlen(buf) >= PAGE_SIZE)			\
> 			break;						\
> 		len += sprintf(page+len, "%s", buf);			\
> 
> We also do not require the NUL-padding behavior that strncpy() provides
> because we are manually setting the entire buffer to NUL, rendering any
> future padding redundant.
> 
> Let's use strscpy() as it guarantees NUL-termination and doesn't
> NUL-pad ( and isn't deprecated :>] ). Note that we can't use the more
> idiomatic strscpy() usage of strscpy(dest, src, sizeof(dest)) because
> the size of the destination buffer is not known to the compiler. We also
> can't use the new 2-arg version of strscpy() from Commit e6584c3964f2f
> ("string: Allow 2-argument strscpy()")
> 
> Link: https://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@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>

Hm, this actually fixes potential over-reads and potential memory content
exposures (in the face of malicious/weird hardware) since p_buf_len
appears to always be sizeof(p_buf) in callers, which means the old use
of strncpy() could have left the string unterminated.

In practice I would assume it's not a problem, but, for example, here's
a place where the 254 p_buf_len could run out when doing the sprintf:

#define VPD_TMP_BUF_SIZE                      254
..
#define INQUIRY_VPD_DEVICE_IDENTIFIER_LEN       254
..
struct t10_vpd {
        unsigned char device_identifier[INQUIRY_VPD_DEVICE_IDENTIFIER_LEN];
	...
};
..
int transport_dump_vpd_ident(..., unsigned char *p_buf, int p_buf_len)
{
	...
        unsigned char buf[VPD_TMP_BUF_SIZE];
	...
                snprintf(buf, sizeof(buf),
                        "T10 VPD ASCII Device Identifier: %s\n",
                        &vpd->device_identifier[0]);
	...
        if (p_buf)
                strncpy(p_buf, buf, p_buf_len);	// may write 254 chars and no NUL
	...
}
..
        unsigned char buf[VPD_TMP_BUF_SIZE];
	...
	memset(buf, 0, VPD_TMP_BUF_SIZE);
        transport_dump_vpd_ident_type(vpd, buf, VPD_TMP_BUF_SIZE);
        if (len + strlen(buf) >= PAGE_SIZE)
		break;
	len += sprintf(page+len, "%s", buf);	// may expose stack memory following "buf"


So, yes, please!

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
> Note: build-tested only.
> 
> Found with: $ rg "strncpy\("
> ---
>  drivers/target/target_core_transport.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 73d0d6133ac8..3311eb87df6d 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1112,7 +1112,7 @@ void transport_dump_vpd_proto_id(
>  	}
>  
>  	if (p_buf)
> -		strncpy(p_buf, buf, p_buf_len);
> +		strscpy(p_buf, buf, p_buf_len);
>  	else
>  		pr_debug("%s", buf);
>  }
> @@ -1162,7 +1162,7 @@ int transport_dump_vpd_assoc(
>  	}
>  
>  	if (p_buf)
> -		strncpy(p_buf, buf, p_buf_len);
> +		strscpy(p_buf, buf, p_buf_len);
>  	else
>  		pr_debug("%s", buf);
>  
> @@ -1222,7 +1222,7 @@ int transport_dump_vpd_ident_type(
>  	if (p_buf) {
>  		if (p_buf_len < strlen(buf)+1)
>  			return -EINVAL;
> -		strncpy(p_buf, buf, p_buf_len);
> +		strscpy(p_buf, buf, p_buf_len);

>  	} else {
>  		pr_debug("%s", buf);
>  	}
> @@ -1276,7 +1276,7 @@ int transport_dump_vpd_ident(
>  	}
>  
>  	if (p_buf)
> -		strncpy(p_buf, buf, p_buf_len);
> +		strscpy(p_buf, buf, p_buf_len);
>  	else
>  		pr_debug("%s", buf);
>  
> 
> ---
> base-commit: bf3a69c6861ff4dc7892d895c87074af7bc1c400
> change-id: 20240318-strncpy-drivers-target-target_core_transport-c-1950554ec04e
> 
> Best regards,
> --
> Justin Stitt <justinstitt@google.com>
> 
> 

-- 
Kees Cook