Re: [PATCH] Revert "arm64/sysreg: refactor deprecated strncpy"

[Marc Zyngier]

On Fri, 01 Sep 2023 12:57:10 +0100,
Mostafa Saleh <smostafa@xxxxxxxxxx> wrote:
> 
> Hi Marc,
> 
> On Fri, Sep 01, 2023 at 12:24:45PM +0100, Marc Zyngier wrote:
> > Hi Mostafa,
> > 
> > On Thu, 31 Aug 2023 17:22:27 +0100,
> > Mostafa Saleh <smostafa@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > >  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?
> I was thinking of something similar, but I see we limit the len anyway
> by the buffer size - 1 and force the NUL at the end so it should be
> safe, unless the goal is to get rid of strncpy all the way, in this
> case we can do it as you proposed.
> 
> There is also a V3 of the original patch which uses memcpy instead.
> https://lore.kernel.org/all/20230831-strncpy-arch-arm64-v3-1-cdbb1e7ea5e1@xxxxxxxxxx/

Ah, that one would actually get my vote, because it does exactly what
I actually intended this piece of code to do.

Justin, do you mind rebasing your v3 on top of to restore the
currently broken functionality?
>
> > 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;
> > 
> > 
> > -- 
> >
> 
> Tested-by: Mostafa Saleh <smostafa@xxxxxxxxxx>

Cheers Mostafa.

You could also just pick it, and repost it as a v2 (I really don't
mind). It should give Will and Catalin a choice of implementations
(and an opportunity for some additional bike-shedding ;-)).

Thanks,

	M.

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