Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Mon, 25 Sep 2023 11:22:29 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Justin Stitt <justinstitt@google.com>
Cc:     Song Liu <song@kernel.org>, linux-raid@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] md: replace deprecated strncpy with memcpy
Message-ID: <202309251122.6E3E678140@keescook>
References: <20230925-strncpy-drivers-md-md-c-v1-1-2b0093b89c2b@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230925-strncpy-drivers-md-md-c-v1-1-2b0093b89c2b@google.com>
Precedence: bulk

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@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>

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

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

-- 
Kees Cook