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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZPHRlmBHZnFAeBeI@google.com>
Date:   Fri, 1 Sep 2023 11:57:10 +0000
From:   Mostafa Saleh <smostafa@...gle.com>
To:     Marc Zyngier <maz@...nel.org>
Cc:     catalin.marinas@....com, will@...nel.org,
        linux-kernel@...r.kernel.org, kvmarm@...ts.linux.dev,
        linux-arm-kernel@...ts.infradead.org, oliver.upton@...ux.dev,
        kristina.martsenko@....com, broonie@...nel.org,
        quic_pkondeti@...cinc.com, justinstitt@...gle.com
Subject: Re: [PATCH] Revert "arm64/sysreg: refactor deprecated strncpy"

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@...gle.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@...gle.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?
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@google.com/

> 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@...gle.com>

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

Thanks,
Mostafa

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ