Messages in this thread | | | Date | Fri, 01 Sep 2023 14:04:29 +0100 | From | Marc Zyngier <> | Subject | Re: [PATCH] Revert "arm64/sysreg: refactor deprecated strncpy" |
| |
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.
| |