linux-hardening - RE: [PATCH] EDAC/mc_sysfs: refactor deprecated strncpy

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]

Date:   Wed, 13 Sep 2023 15:13:14 +0000
From:   "Luck, Tony" <tony.luck@...el.com>
To:     Justin Stitt <justinstitt@...gle.com>,
        Borislav Petkov <bp@...en8.de>,
        James Morse <james.morse@....com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Robert Richter <rric@...nel.org>
CC:     "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org>
Subject: RE: [PATCH] EDAC/mc_sysfs: refactor deprecated strncpy

> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> We should prefer more robust and less ambiguous string interfaces.
>
> A suitable replacement is `strscpy_pad` [2] due to the fact that it guarantees
> NUL-termination on the destination buffer whilst maintaining the
> NUL-padding behavior that `strncpy` provides. This may not be strictly
> necessary but as I couldn't understand what this code does I wanted to
> ensure that the functionality is the same.
>
> 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@...r.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@...gle.com>
> ---
> Note: build-tested only.
> ---
>  drivers/edac/edac_mc_sysfs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index 15f63452a9be..b303309a63cf 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -229,8 +229,7 @@ static ssize_t channel_dimm_label_store(struct device *dev,
>         if (copy_count == 0 || copy_count >= sizeof(rank->dimm->label))
>                 return -EINVAL;
>
> -       strncpy(rank->dimm->label, data, copy_count);
> -       rank->dimm->label[copy_count] = '\0';
> +       strscpy_pad(rank->dimm->label, data, copy_count);

That doc page says the problem with strncpy() is that it doesn't guarantee to
NUL terminate the target string. But this code is aware of that limitation and
zaps a '\0' at the end to be sure.

So this code doesn't suffer from the potential problems.

If it is going to be fixed, then some further analysis of the original code
would be wise. Just replacing with strscpy_pad() means the code probably
still suffers from the "needless performance penalty" also mentioned in
the deprecation document.

-Tony