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: <202402211630.184FE822FB@keescook>
Date: Wed, 21 Feb 2024 16:30:32 -0800
From: Kees Cook <keescook@...omium.org>
To: Justin Stitt <justinstitt@...gle.com>
Cc: Hannes Reinecke <hare@...e.de>,
	"James E.J. Bottomley" <jejb@...ux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@...cle.com>,
	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2] scsi: libfc: replace deprecated strncpy with memcpy

On Wed, Feb 21, 2024 at 11:50:26PM +0000, Justin Stitt wrote:
> strncpy() is deprecated [1] and as such we should use different apis to
> copy string data.
> 
> We can see that ct is NUL-initialized with fc_ct_hdr_fill:
> |       ct = fc_ct_hdr_fill(fp, op, sizeof(struct fc_ns_rspn) + len,
> ...
> 
> In fc_ct_hdr_fill():
> |       memset(ct, 0, ct_plen);
> 
> We also calculate the length of the source string:
> |       len = strnlen(fc_host_symbolic_name(lport->host), 255);
> 
> ...then this argument is used in strncpy(), which is bad because the
> pattern of (dest, src, strlen(src)) usually leaves the destination
> buffer without NUL-termination. However, it looks as though we do not
> require NUL-termination since fr_name is part of a seq_buf-like
> structure wherein its length is monitored:
> |       struct fc_ns_rspn {
> |       	struct fc_ns_fid fr_fid;	/* port ID object */
> |       	__u8		fr_name_len;
> |       	char		fr_name[];
> |       } __attribute__((__packed__));
> 
> So, this is really just a byte copy into a length-bounded buffer. Let's
> use memcpy().
> 
> 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@...r.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@...gle.com>

Thanks for the refresh! This looks right to me.

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

-- 
Kees Cook

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ