Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 1 Aug 2023 16:33:29 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Justin Stitt <justinstitt@google.com>
Cc:     Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
        Nick Desaulniers <ndesaulniers@google.com>,
        linux-hardening@vger.kernel.org
Subject: Re: [PATCH] RISC-V: cpu: refactor deprecated strncpy
Message-ID: <202308011631.BDCD5CE33A@keescook>
References: <20230801-arch-riscv-kernel-v1-1-2b3f2dc0bc61@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230801-arch-riscv-kernel-v1-1-2b3f2dc0bc61@google.com>
Precedence: bulk

On Tue, Aug 01, 2023 at 09:14:56PM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> 
> A suitable replacement is `strscpy` [2] due to the fact that it
> guarantees NUL-termination on its destination buffer argument which is
> _not_ the case for `strncpy`!
> 
> The `sv_type` buffer is declared with a size of 16 which is then
> followed by some `strncpy` calls to populate the buffer with one of:
> "sv32", "sv57", "sv48", "sv39" or "none". Hard-coding the max length as 5 is
> error-prone and involves counting the number of characters (and
> hopefully not forgetting to count the NUL-byte) in the raw string.
> 
> Using a pre-determined max length in combination with `strscpy` provides
> a cleaner, less error-prone as well as a less ambiguous implementation.
> `strscpy` guarantees that it's destination buffer is NUL-terminated even
> if it's source argument exceeds the max length as defined by the third
> argument.
> 
> To be clear, there is no bug (i think?) in the current implementation
> but the current hard-coded values in combination with using a deprecated
> interface make this a worthwhile change, IMO.
> 
> [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> 
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
>  arch/riscv/kernel/cpu.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index a2fc952318e9..1c576e4ec171 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -17,6 +17,8 @@
>  #include <asm/smp.h>
>  #include <asm/pgtable.h>
>  
> +#define SV_TYPE_MAX_LENGTH 16
> +
>  /*
>   * Returns the hart ID of the given device tree node, or -ENODEV if the node
>   * isn't an enabled and valid RISC-V hart node.
> @@ -271,21 +273,21 @@ static void print_isa(struct seq_file *f, const char *isa)
>  
>  static void print_mmu(struct seq_file *f)
>  {
> -	char sv_type[16];
> +	char sv_type[SV_TYPE_MAX_LENGTH];
>  
>  #ifdef CONFIG_MMU
>  #if defined(CONFIG_32BIT)
> -	strncpy(sv_type, "sv32", 5);
> +	strscpy(sv_type, "sv32", SV_TYPE_MAX_LENGTH);
>  #elif defined(CONFIG_64BIT)
>  	if (pgtable_l5_enabled)
> -		strncpy(sv_type, "sv57", 5);
> +		strscpy(sv_type, "sv57", SV_TYPE_MAX_LENGTH);
>  	else if (pgtable_l4_enabled)
> -		strncpy(sv_type, "sv48", 5);
> +		strscpy(sv_type, "sv48", SV_TYPE_MAX_LENGTH);
>  	else
> -		strncpy(sv_type, "sv39", 5);
> +		strscpy(sv_type, "sv39", SV_TYPE_MAX_LENGTH);
>  #endif
>  #else
> -	strncpy(sv_type, "none", 5);
> +	strscpy(sv_type, "none", SV_TYPE_MAX_LENGTH);
>  #endif /* CONFIG_MMU */
>  	seq_printf(f, "mmu\t\t: %s\n", sv_type);
>  }

I'd say just throw the whole buffer away and just avoid copying the
.rodata strings onto the stack for no reason. They can be used directly:

static void print_mmu(struct seq_file *f)
{
        const char *sv_type;

#ifdef CONFIG_MMU
#if defined(CONFIG_32BIT)
	sv_type = "sv32";
#elif defined(CONFIG_64BIT)
	if (pgtable_l5_enabled)
		sv_type = "sv57";
	else if (pgtable_l4_enabled)
		sv_type = "sv48";
	else
		sv_type = "sv39";
#endif
#else
	sv_type = "none";
#endif /* CONFIG_MMU */
	seq_printf(f, "mmu\t\t: %s\n", sv_type);
}

-- 
Kees Cook