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:   Fri, 01 Sep 2023 12:24:45 +0100
Message-ID: <86v8cudtky.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Mostafa Saleh <smostafa@google.com>
Cc:     catalin.marinas@arm.com, will@kernel.org,
        linux-kernel@vger.kernel.org, kvmarm@lists.linux.dev,
        linux-arm-kernel@lists.infradead.org, oliver.upton@linux.dev,
        kristina.martsenko@arm.com, broonie@kernel.org,
        quic_pkondeti@quicinc.com, justinstitt@google.com
Subject: Re: [PATCH] Revert "arm64/sysreg: refactor deprecated strncpy"
In-Reply-To: <20230831162227.2307863-1-smostafa@google.com>
References: <20230831162227.2307863-1-smostafa@google.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

Hi Mostafa,

On Thu, 31 Aug 2023 17:22:27 +0100,
Mostafa Saleh <smostafa@google.com> wrote:
> 
> This reverts commit d232606773a0b09ec7f1ffc25f63abe801d011fd.
> 
> Using strscpy is not correct in this context and the commit
> assumption is not right "strncpy is deprecated for use on
> NUL-terminated destination strings".
> 
> strncpy is used here to copy parts of the string(cmdline) separated
> by spaces into the buffer and not a NULL terminated string.
> 
> This breaks the arm options "kvm-arm.mode=protected, arm64.nobti ..."
> 
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  arch/arm64/kernel/idreg-override.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c
> index aee12c75b738..2fe2491b692c 100644
> --- a/arch/arm64/kernel/idreg-override.c
> +++ b/arch/arm64/kernel/idreg-override.c
> @@ -262,9 +262,9 @@ static __init void __parse_cmdline(const char *cmdline, bool parse_aliases)
>  		if (!len)
>  			return;
>  
> -		len = strscpy(buf, cmdline, ARRAY_SIZE(buf));
> -		if (len == -E2BIG)
> -			len = ARRAY_SIZE(buf) - 1;
> +		len = min(len, ARRAY_SIZE(buf) - 1);
> +		strncpy(buf, cmdline, len);
> +		buf[len] = 0;
>  
>  		if (strcmp(buf, "--") == 0)
>  			return;

Instead of completely reverting the patch, maybe something like the
hack below (completely untested), so that we can still get rid of
another instance of strncpy(), and yet bring back some sanity in the
logic?

Thanks,

	M.

diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c
index aee12c75b738..be5c778a3f14 100644
--- a/arch/arm64/kernel/idreg-override.c
+++ b/arch/arm64/kernel/idreg-override.c
@@ -262,9 +262,8 @@ static __init void __parse_cmdline(const char *cmdline, bool parse_aliases)
 		if (!len)
 			return;
 
-		len = strscpy(buf, cmdline, ARRAY_SIZE(buf));
-		if (len == -E2BIG)
-			len = ARRAY_SIZE(buf) - 1;
+		len = min(len, ARRAY_SIZE(buf) - 1);
+		strscpy(buf, cmdline, len);
 
 		if (strcmp(buf, "--") == 0)
 			return;


-- 
Without deviation from the norm, progress is not possible.