Message-ID: <CAHVXubgzac0gXNF2FVeUrCAnOe7U9QhAfj3nWd_jc0maaepN2g@mail.gmail.com>
Date:   Wed, 21 Dec 2022 15:23:36 +0100
From:   Alexandre Ghiti <alexghiti@...osinc.com>
To:     Conor Dooley <conor@...nel.org>
Cc:     Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Andrey Ryabinin <ryabinin.a.a@...il.com>,
        Alexander Potapenko <glider@...gle.com>,
        Andrey Konovalov <andreyknvl@...il.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Vincenzo Frascino <vincenzo.frascino@....com>,
        Ard Biesheuvel <ardb@...nel.org>,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
        kasan-dev@...glegroups.com, linux-efi@...r.kernel.org
Subject: Re: [PATCH 4/6] riscv: Fix EFI stub usage of KASAN instrumented
 string functions

Hi Conor,

On Wed, Dec 21, 2022 at 3:06 PM Conor Dooley <conor@...nel.org> wrote:
>
> Hey Alex!
>
> On Fri, Dec 16, 2022 at 05:21:39PM +0100, Alexandre Ghiti wrote:
> > The EFI stub must not use any KASAN instrumented code as the kernel
> > proper did not initialize the thread pointer and the mapping for the
> > KASAN shadow region.
> >
> > Avoid using generic string functions by copying stub dependencies from
> > lib/string.c to drivers/firmware/efi/libstub/string.c as RISC-V does
> > not implement architecture-specific versions of those functions.
>
> To the unaware among us, how does this interact with Heiko's custom
> functions for bitmanip extensions? Is this diametrically opposed to
> that, or does it actually help avoid having to have special handling
> for the efi stub?

I'm not sure which patchset you are referring to, but I guess you are
talking about arch-specific string functions:

- If they are written in assembly and are then not kasan-instrumented,
we'll be able to use them and then revert part of this patch.
- If they are written in C and are then kasan-instrumented (because
we'll want to instrument them), we'll keep using the implementation
added here.

Hope that answers your question!

Alex

>
> Also, checkpatch seems to be rather unhappy with you here:
> https://gist.github.com/conor-pwbot/e5b4c8f2c3b88b4a8fcab4df437613e2

Yes, those new functions are exact copies from lib/string.c, I did not
want to fix those checkpatch errors in this patchset.

>
> Thanks,
> Conor.
>
> >
> > Signed-off-by: Alexandre Ghiti <alexghiti@...osinc.com>
> > ---
> >  arch/riscv/kernel/image-vars.h        |   8 --
> >  drivers/firmware/efi/libstub/Makefile |   7 +-
> >  drivers/firmware/efi/libstub/string.c | 133 ++++++++++++++++++++++++++
> >  3 files changed, 137 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/image-vars.h b/arch/riscv/kernel/image-vars.h
> > index d6e5f739905e..15616155008c 100644
> > --- a/arch/riscv/kernel/image-vars.h
> > +++ b/arch/riscv/kernel/image-vars.h
> > @@ -23,14 +23,6 @@
> >   * linked at. The routines below are all implemented in assembler in a
> >   * position independent manner
> >   */
> > -__efistub_memcmp             = memcmp;
> > -__efistub_memchr             = memchr;
> > -__efistub_strlen             = strlen;
> > -__efistub_strnlen            = strnlen;
> > -__efistub_strcmp             = strcmp;
> > -__efistub_strncmp            = strncmp;
> > -__efistub_strrchr            = strrchr;
> > -
> >  __efistub__start             = _start;
> >  __efistub__start_kernel              = _start_kernel;
> >  __efistub__end                       = _end;
> > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > index b1601aad7e1a..031d2268bab5 100644
> > --- a/drivers/firmware/efi/libstub/Makefile
> > +++ b/drivers/firmware/efi/libstub/Makefile
> > @@ -130,9 +130,10 @@ STUBCOPY_RELOC-$(CONFIG_ARM)     := R_ARM_ABS
> >  # also means that we need to be extra careful to make sure that the stub does
> >  # not rely on any absolute symbol references, considering that the virtual
> >  # kernel mapping that the linker uses is not active yet when the stub is
> > -# executing. So build all C dependencies of the EFI stub into libstub, and do
> > -# a verification pass to see if any absolute relocations exist in any of the
> > -# object files.
> > +# executing. In addition, we need to make sure that the stub does not use KASAN
> > +# instrumented code like the generic string functions. So build all C
> > +# dependencies of the EFI stub into libstub, and do a verification pass to see
> > +# if any absolute relocations exist in any of the object files.
> >  #
> >  STUBCOPY_FLAGS-$(CONFIG_ARM64)       += --prefix-alloc-sections=.init \
> >                                  --prefix-symbols=__efistub_
> > diff --git a/drivers/firmware/efi/libstub/string.c b/drivers/firmware/efi/libstub/string.c
> > index 5d13e43869ee..5154ae6e7f10 100644
> > --- a/drivers/firmware/efi/libstub/string.c
> > +++ b/drivers/firmware/efi/libstub/string.c
> > @@ -113,3 +113,136 @@ long simple_strtol(const char *cp, char **endp, unsigned int base)
> >
> >       return simple_strtoull(cp, endp, base);
> >  }
> > +
> > +#ifndef __HAVE_ARCH_STRLEN
> > +/**
> > + * strlen - Find the length of a string
> > + * @s: The string to be sized
> > + */
> > +size_t strlen(const char *s)
> > +{
> > +     const char *sc;
> > +
> > +     for (sc = s; *sc != '\0'; ++sc)
> > +             /* nothing */;
> > +     return sc - s;
> > +}
> > +EXPORT_SYMBOL(strlen);
> > +#endif
> > +
> > +#ifndef __HAVE_ARCH_STRNLEN
> > +/**
> > + * strnlen - Find the length of a length-limited string
> > + * @s: The string to be sized
> > + * @count: The maximum number of bytes to search
> > + */
> > +size_t strnlen(const char *s, size_t count)
> > +{
> > +     const char *sc;
> > +
> > +     for (sc = s; count-- && *sc != '\0'; ++sc)
> > +             /* nothing */;
> > +     return sc - s;
> > +}
> > +EXPORT_SYMBOL(strnlen);
> > +#endif
> > +
> > +#ifndef __HAVE_ARCH_STRCMP
> > +/**
> > + * strcmp - Compare two strings
> > + * @cs: One string
> > + * @ct: Another string
> > + */
> > +int strcmp(const char *cs, const char *ct)
> > +{
> > +     unsigned char c1, c2;
> > +
> > +     while (1) {
> > +             c1 = *cs++;
> > +             c2 = *ct++;
> > +             if (c1 != c2)
> > +                     return c1 < c2 ? -1 : 1;
> > +             if (!c1)
> > +                     break;
> > +     }
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(strcmp);
> > +#endif
> > +
> > +#ifndef __HAVE_ARCH_STRRCHR
> > +/**
> > + * strrchr - Find the last occurrence of a character in a string
> > + * @s: The string to be searched
> > + * @c: The character to search for
> > + */
> > +char *strrchr(const char *s, int c)
> > +{
> > +     const char *last = NULL;
> > +     do {
> > +             if (*s == (char)c)
> > +                     last = s;
> > +     } while (*s++);
> > +     return (char *)last;
> > +}
> > +EXPORT_SYMBOL(strrchr);
> > +#endif
> > +
> > +#ifndef __HAVE_ARCH_MEMCMP
> > +/**
> > + * memcmp - Compare two areas of memory
> > + * @cs: One area of memory
> > + * @ct: Another area of memory
> > + * @count: The size of the area.
> > + */
> > +#undef memcmp
> > +__visible int memcmp(const void *cs, const void *ct, size_t count)
> > +{
> > +     const unsigned char *su1, *su2;
> > +     int res = 0;
> > +
> > +#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> > +     if (count >= sizeof(unsigned long)) {
> > +             const unsigned long *u1 = cs;
> > +             const unsigned long *u2 = ct;
> > +             do {
> > +                     if (get_unaligned(u1) != get_unaligned(u2))
> > +                             break;
> > +                     u1++;
> > +                     u2++;
> > +                     count -= sizeof(unsigned long);
> > +             } while (count >= sizeof(unsigned long));
> > +             cs = u1;
> > +             ct = u2;
> > +     }
> > +#endif
> > +     for (su1 = cs, su2 = ct; 0 < count; ++su1, ++su2, count--)
> > +             if ((res = *su1 - *su2) != 0)
> > +                     break;
> > +     return res;
> > +}
> > +EXPORT_SYMBOL(memcmp);
> > +#endif
> > +
> > +#ifndef __HAVE_ARCH_MEMCHR
> > +/**
> > + * memchr - Find a character in an area of memory.
> > + * @s: The memory area
> > + * @c: The byte to search for
> > + * @n: The size of the area.
> > + *
> > + * returns the address of the first occurrence of @c, or %NULL
> > + * if @c is not found
> > + */
> > +void *memchr(const void *s, int c, size_t n)
> > +{
> > +     const unsigned char *p = s;
> > +     while (n-- != 0) {
> > +             if ((unsigned char)c == *p++) {
> > +                     return (void *)(p - 1);
> > +             }
> > +     }
> > +     return NULL;
> > +}
> > +EXPORT_SYMBOL(memchr);
> > +#endif
> > --
> > 2.37.2
> >
> >

Powered by blists - more mailing lists