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@xxxxxxxxxxxxxxx
> Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx>
I agree on the analysis of the replacements. Thanks for all the detail!
Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
--
Kees Cook
