linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Revert "arm64/sysreg: refactor deprecated strncpy"
@ 2023-08-31 16:22 Mostafa Saleh
  2023-09-01 11:24 ` Marc Zyngier
  0 siblings, 1 reply; 4+ messages in thread
From: Mostafa Saleh @ 2023-08-31 16:22 UTC (permalink / raw)
  To: catalin.marinas, will, linux-kernel, kvmarm, linux-arm-kernel
  Cc: maz, oliver.upton, kristina.martsenko, broonie, quic_pkondeti,
	smostafa, justinstitt

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;
-- 
2.42.0.rc2.253.gd59a3bf2b4-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] Revert "arm64/sysreg: refactor deprecated strncpy"
  2023-08-31 16:22 [PATCH] Revert "arm64/sysreg: refactor deprecated strncpy" Mostafa Saleh
@ 2023-09-01 11:24 ` Marc Zyngier
  2023-09-01 11:57   ` Mostafa Saleh
  0 siblings, 1 reply; 4+ messages in thread
From: Marc Zyngier @ 2023-09-01 11:24 UTC (permalink / raw)
  To: Mostafa Saleh
  Cc: catalin.marinas, will, linux-kernel, kvmarm, linux-arm-kernel,
	oliver.upton, kristina.martsenko, broonie, quic_pkondeti,
	justinstitt

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] Revert "arm64/sysreg: refactor deprecated strncpy"
  2023-09-01 11:24 ` Marc Zyngier
@ 2023-09-01 11:57   ` Mostafa Saleh
  2023-09-01 13:04     ` Marc Zyngier
  0 siblings, 1 reply; 4+ messages in thread
From: Mostafa Saleh @ 2023-09-01 11:57 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: catalin.marinas, will, linux-kernel, kvmarm, linux-arm-kernel,
	oliver.upton, kristina.martsenko, broonie, quic_pkondeti,
	justinstitt

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

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

Thanks,
Mostafa

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] Revert "arm64/sysreg: refactor deprecated strncpy"
  2023-09-01 11:57   ` Mostafa Saleh
@ 2023-09-01 13:04     ` Marc Zyngier
  0 siblings, 0 replies; 4+ messages in thread
From: Marc Zyngier @ 2023-09-01 13:04 UTC (permalink / raw)
  To: Mostafa Saleh, justinstitt
  Cc: catalin.marinas, will, linux-kernel, kvmarm, linux-arm-kernel,
	oliver.upton, kristina.martsenko, broonie, quic_pkondeti

On Fri, 01 Sep 2023 12:57:10 +0100,
Mostafa Saleh <smostafa@google.com> 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@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?
> 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/

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

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-09-01 13:05 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-31 16:22 [PATCH] Revert "arm64/sysreg: refactor deprecated strncpy" Mostafa Saleh
2023-09-01 11:24 ` Marc Zyngier
2023-09-01 11:57   ` Mostafa Saleh
2023-09-01 13:04     ` Marc Zyngier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).