On Fri, 01 Sep 2023 12:57:10 +0100,
Mostafa Saleh <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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/[email protected]/
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 <[email protected]>
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.