linux-hardening - Re: [PATCH] md: replace deprecated strncpy with memcpy

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: <202309251122.6E3E678140@keescook>
Date: Mon, 25 Sep 2023 11:22:29 -0700
From: Kees Cook <keescook@...omium.org>
To: Justin Stitt <justinstitt@...gle.com>
Cc: Song Liu <song@...nel.org>, linux-raid@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] md: replace deprecated strncpy with memcpy

On Mon, Sep 25, 2023 at 09:49:17AM +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.
> 
> There are three such strncpy uses that this patch addresses:
> 
> The respective destination buffers are:
> 1) mddev->clevel
> 2) clevel
> 3) mddev->metadata_type
> 
> We expect mddev->clevel to be NUL-terminated due to its use with format
> strings:
> |       ret = sprintf(page, "%s\n", mddev->clevel);
> 
> Furthermore, we can see that mddev->clevel is not expected to be
> NUL-padded as `md_clean()` merely set's its first byte to NULL -- not
> the entire buffer:
> |       static void md_clean(struct mddev *mddev)
> |       {
> |       	mddev->array_sectors = 0;
> |       	mddev->external_size = 0;
> |               ...
> |       	mddev->level = LEVEL_NONE;
> |       	mddev->clevel[0] = 0;
> |               ...
> 
> A suitable replacement for this instance is `memcpy` as we know the
> number of bytes to copy and perform manual NUL-termination at a
> specified offset. This really decays to just a byte copy from one buffer
> to another. `strscpy` is also a considerable replacement but using
> `slen` as the length argument would result in truncation of the last
> byte unless something like `slen + 1` was provided which isn't the most
> idiomatic strscpy usage.
> 
> For the next case, the destination buffer `clevel` is expected to be
> NUL-terminated based on its usage within kstrtol() which expects
> NUL-terminated strings. Note that, in context, this code removes a
> trailing newline which is seemingly not required as kstrtol() can handle
> trailing newlines implicitly. However, there exists further usage of
> clevel (or buf) that would also like to have the newline removed. All in
> all, with similar reasoning to the first case, let's just use memcpy as
> this is just a byte copy and NUL-termination is handled manually.
> 
> The third and final case concerning `mddev->metadata_type` is more or
> less the same as the other two. We expect that it be NUL-terminated
> based on its usage with seq_printf:
> |       seq_printf(seq, " super external:%s",
> |       	   mddev->metadata_type);
> ... and we can surmise that NUL-padding isn't required either due to how
> it is handled in md_clean():
> |       static void md_clean(struct mddev *mddev)
> |       {
> |       ...
> |       mddev->metadata_type[0] = 0;
> |       ...
> 
> So really, all these instances have precisely calculated lengths and
> purposeful NUL-termination so we can just use memcpy to remove ambiguity
> surrounding strncpy.
> 
> 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>

I agree on the analysis of the replacements. Thanks for all the detail!

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

-- 
Kees Cook