[PATCH] lkdtm: fix memory copy size for WRITE_KERN

[PATCH] lkdtm: fix memory copy size for WRITE_KERN

[Candle Sun]

From: Candle Sun <candle.sun@unisoc.com>

Though do_overwritten() follows do_nothing() in source code, the final
memory address order is determined by compiler. We can't always assume
address of do_overwritten() is bigger than do_nothing(). At least the
Clang we are using places do_overwritten() before do_nothing() in the
object. This causes the copy size in lkdtm_WRITE_KERN() is *really*
big and WRITE_KERN test on ARM32 arch will fail.

Compare the address order before doing the subtraction.

Signed-off-by: Candle Sun <candle.sun@unisoc.com>
---
 drivers/misc/lkdtm/perms.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
index 2dede2ef658f..fbfbdf89d668 100644
--- a/drivers/misc/lkdtm/perms.c
+++ b/drivers/misc/lkdtm/perms.c
@@ -31,13 +31,13 @@ static unsigned long ro_after_init __ro_after_init = 0x55AA5500;
  * This just returns to the caller. It is designed to be copied into
  * non-executable memory regions.
  */
-static void do_nothing(void)
+static noinline void do_nothing(void)
 {
 	return;
 }
 
 /* Must immediately follow do_nothing for size calculuations to work out. */
-static void do_overwritten(void)
+static noinline void do_overwritten(void)
 {
 	pr_info("do_overwritten wasn't overwritten!\n");
 	return;
@@ -110,15 +110,14 @@ void lkdtm_WRITE_RO_AFTER_INIT(void)
 
 void lkdtm_WRITE_KERN(void)
 {
-	size_t size;
-	volatile unsigned char *ptr;
+	unsigned long value_dow = (unsigned long)do_overwritten;
+	unsigned long value_do =  (unsigned long)do_nothing;
+	size_t size = (size_t)(value_dow > value_do ?
+			value_dow - value_do : value_do - value_dow);
 
-	size = (unsigned long)do_overwritten - (unsigned long)do_nothing;
-	ptr = (unsigned char *)do_overwritten;
-
-	pr_info("attempting bad %zu byte write at %px\n", size, ptr);
-	memcpy((void *)ptr, (unsigned char *)do_nothing, size);
-	flush_icache_range((unsigned long)ptr, (unsigned long)(ptr + size));
+	pr_info("attempting bad %zu byte write at %px\n", size, do_overwritten);
+	memcpy((void *)value_dow, (void *)value_do, size);
+	flush_icache_range(value_dow, value_dow + (unsigned long)size);
 	pr_err("FAIL: survived bad write\n");
 
 	do_overwritten();
-- 
2.17.0

RE: [PATCH] lkdtm: fix memory copy size for WRITE_KERN

[David Laight]

From: Candle Sun
> Sent: 25 January 2021 08:56
> 
> From: Candle Sun <candle.sun@unisoc.com>
> 
> Though do_overwritten() follows do_nothing() in source code, the final
> memory address order is determined by compiler. We can't always assume
> address of do_overwritten() is bigger than do_nothing(). At least the
> Clang we are using places do_overwritten() before do_nothing() in the
> object. This causes the copy size in lkdtm_WRITE_KERN() is *really*
> big and WRITE_KERN test on ARM32 arch will fail.
> 
> Compare the address order before doing the subtraction.

It isn't clear that helps.
Compile with -ffunction-sections and/or do LTO an there
is no reason at all why the functions should be together.

Even without that lkdtm_WRITE_KERN() could easily be between them.

You need to get the size of the 'empty function' from the
symbol table.

	David

> 
> Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> ---
>  drivers/misc/lkdtm/perms.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 2dede2ef658f..fbfbdf89d668 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -31,13 +31,13 @@ static unsigned long ro_after_init __ro_after_init = 0x55AA5500;
>   * This just returns to the caller. It is designed to be copied into
>   * non-executable memory regions.
>   */
> -static void do_nothing(void)
> +static noinline void do_nothing(void)
>  {
>  	return;
>  }
> 
>  /* Must immediately follow do_nothing for size calculuations to work out. */
> -static void do_overwritten(void)
> +static noinline void do_overwritten(void)
>  {
>  	pr_info("do_overwritten wasn't overwritten!\n");
>  	return;
> @@ -110,15 +110,14 @@ void lkdtm_WRITE_RO_AFTER_INIT(void)
> 
>  void lkdtm_WRITE_KERN(void)
>  {
> -	size_t size;
> -	volatile unsigned char *ptr;
> +	unsigned long value_dow = (unsigned long)do_overwritten;
> +	unsigned long value_do =  (unsigned long)do_nothing;
> +	size_t size = (size_t)(value_dow > value_do ?
> +			value_dow - value_do : value_do - value_dow);
> 
> -	size = (unsigned long)do_overwritten - (unsigned long)do_nothing;
> -	ptr = (unsigned char *)do_overwritten;
> -
> -	pr_info("attempting bad %zu byte write at %px\n", size, ptr);
> -	memcpy((void *)ptr, (unsigned char *)do_nothing, size);
> -	flush_icache_range((unsigned long)ptr, (unsigned long)(ptr + size));
> +	pr_info("attempting bad %zu byte write at %px\n", size, do_overwritten);
> +	memcpy((void *)value_dow, (void *)value_do, size);
> +	flush_icache_range(value_dow, value_dow + (unsigned long)size);
>  	pr_err("FAIL: survived bad write\n");
> 
>  	do_overwritten();
> --
> 2.17.0

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] lkdtm: fix memory copy size for WRITE_KERN

[Nick Desaulniers]

On Mon, Jan 25, 2021 at 12:56 AM Candle Sun <candlesea@gmail.com> wrote:
>
> From: Candle Sun <candle.sun@unisoc.com>
>
> Though do_overwritten() follows do_nothing() in source code, the final
> memory address order is determined by compiler. We can't always assume
> address of do_overwritten() is bigger than do_nothing(). At least the
> Clang we are using places do_overwritten() before do_nothing() in the
> object. This causes the copy size in lkdtm_WRITE_KERN() is *really*
> big and WRITE_KERN test on ARM32 arch will fail.
>
> Compare the address order before doing the subtraction.
>
> Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> ---
>  drivers/misc/lkdtm/perms.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 2dede2ef658f..fbfbdf89d668 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -31,13 +31,13 @@ static unsigned long ro_after_init __ro_after_init = 0x55AA5500;
>   * This just returns to the caller. It is designed to be copied into
>   * non-executable memory regions.
>   */
> -static void do_nothing(void)
> +static noinline void do_nothing(void)
>  {
>         return;
>  }
>
>  /* Must immediately follow do_nothing for size calculuations to work out. */
> -static void do_overwritten(void)
> +static noinline void do_overwritten(void)
>  {
>         pr_info("do_overwritten wasn't overwritten!\n");
>         return;
> @@ -110,15 +110,14 @@ void lkdtm_WRITE_RO_AFTER_INIT(void)
>
>  void lkdtm_WRITE_KERN(void)
>  {
> -       size_t size;
> -       volatile unsigned char *ptr;
> +       unsigned long value_dow = (unsigned long)do_overwritten;
> +       unsigned long value_do =  (unsigned long)do_nothing;
> +       size_t size = (size_t)(value_dow > value_do ?
> +                       value_dow - value_do : value_do - value_dow);
>
> -       size = (unsigned long)do_overwritten - (unsigned long)do_nothing;

Thanks for the patch! Might it be simpler to do:

size = abs((uintptr_t)do_overwritten - (uintptr_t)do_nothing));

Then I don't think much of this function needs to be changed.

> -       ptr = (unsigned char *)do_overwritten;
> -
> -       pr_info("attempting bad %zu byte write at %px\n", size, ptr);
> -       memcpy((void *)ptr, (unsigned char *)do_nothing, size);
> -       flush_icache_range((unsigned long)ptr, (unsigned long)(ptr + size));
> +       pr_info("attempting bad %zu byte write at %px\n", size, do_overwritten);
> +       memcpy((void *)value_dow, (void *)value_do, size);
> +       flush_icache_range(value_dow, value_dow + (unsigned long)size);
>         pr_err("FAIL: survived bad write\n");
>
>         do_overwritten();
> --
> 2.17.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] lkdtm: fix memory copy size for WRITE_KERN

[Candle Sun]

On Mon, Jan 25, 2021 at 6:37 PM David Laight <David.Laight@aculab.com> wrote:
>
> From: Candle Sun
> > Sent: 25 January 2021 08:56
> >
> > From: Candle Sun <candle.sun@unisoc.com>
> >
> > Though do_overwritten() follows do_nothing() in source code, the final
> > memory address order is determined by compiler. We can't always assume
> > address of do_overwritten() is bigger than do_nothing(). At least the
> > Clang we are using places do_overwritten() before do_nothing() in the
> > object. This causes the copy size in lkdtm_WRITE_KERN() is *really*
> > big and WRITE_KERN test on ARM32 arch will fail.
> >
> > Compare the address order before doing the subtraction.
>
> It isn't clear that helps.
> Compile with -ffunction-sections and/or do LTO an there
> is no reason at all why the functions should be together.
>
> Even without that lkdtm_WRITE_KERN() could easily be between them.
>
> You need to get the size of the 'empty function' from the
> symbol table.
>
>         David

Thanks David.

I think using abs() by Nick's advice would be better. But could you
point out which kernel function can get function size?

Regards,
Candle


>
> >
> > Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> > ---
> >  drivers/misc/lkdtm/perms.c | 19 +++++++++----------
> >  1 file changed, 9 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> > index 2dede2ef658f..fbfbdf89d668 100644
> > --- a/drivers/misc/lkdtm/perms.c
> > +++ b/drivers/misc/lkdtm/perms.c
> > @@ -31,13 +31,13 @@ static unsigned long ro_after_init __ro_after_init = 0x55AA5500;
> >   * This just returns to the caller. It is designed to be copied into
> >   * non-executable memory regions.
> >   */
> > -static void do_nothing(void)
> > +static noinline void do_nothing(void)
> >  {
> >       return;
> >  }
> >
> >  /* Must immediately follow do_nothing for size calculuations to work out. */
> > -static void do_overwritten(void)
> > +static noinline void do_overwritten(void)
> >  {
> >       pr_info("do_overwritten wasn't overwritten!\n");
> >       return;
> > @@ -110,15 +110,14 @@ void lkdtm_WRITE_RO_AFTER_INIT(void)
> >
> >  void lkdtm_WRITE_KERN(void)
> >  {
> > -     size_t size;
> > -     volatile unsigned char *ptr;
> > +     unsigned long value_dow = (unsigned long)do_overwritten;
> > +     unsigned long value_do =  (unsigned long)do_nothing;
> > +     size_t size = (size_t)(value_dow > value_do ?
> > +                     value_dow - value_do : value_do - value_dow);
> >
> > -     size = (unsigned long)do_overwritten - (unsigned long)do_nothing;
> > -     ptr = (unsigned char *)do_overwritten;
> > -
> > -     pr_info("attempting bad %zu byte write at %px\n", size, ptr);
> > -     memcpy((void *)ptr, (unsigned char *)do_nothing, size);
> > -     flush_icache_range((unsigned long)ptr, (unsigned long)(ptr + size));
> > +     pr_info("attempting bad %zu byte write at %px\n", size, do_overwritten);
> > +     memcpy((void *)value_dow, (void *)value_do, size);
> > +     flush_icache_range(value_dow, value_dow + (unsigned long)size);
> >       pr_err("FAIL: survived bad write\n");
> >
> >       do_overwritten();
> > --
> > 2.17.0
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>

Re: [PATCH] lkdtm: fix memory copy size for WRITE_KERN

[Candle Sun]

On Tue, Jan 26, 2021 at 5:16 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Mon, Jan 25, 2021 at 12:56 AM Candle Sun <candlesea@gmail.com> wrote:
> >
> > From: Candle Sun <candle.sun@unisoc.com>
> >
> > Though do_overwritten() follows do_nothing() in source code, the final
> > memory address order is determined by compiler. We can't always assume
> > address of do_overwritten() is bigger than do_nothing(). At least the
> > Clang we are using places do_overwritten() before do_nothing() in the
> > object. This causes the copy size in lkdtm_WRITE_KERN() is *really*
> > big and WRITE_KERN test on ARM32 arch will fail.
> >
> > Compare the address order before doing the subtraction.
> >
> > Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> > ---
> >  drivers/misc/lkdtm/perms.c | 19 +++++++++----------
> >  1 file changed, 9 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> > index 2dede2ef658f..fbfbdf89d668 100644
> > --- a/drivers/misc/lkdtm/perms.c
> > +++ b/drivers/misc/lkdtm/perms.c
> > @@ -31,13 +31,13 @@ static unsigned long ro_after_init __ro_after_init = 0x55AA5500;
> >   * This just returns to the caller. It is designed to be copied into
> >   * non-executable memory regions.
> >   */
> > -static void do_nothing(void)
> > +static noinline void do_nothing(void)
> >  {
> >         return;
> >  }
> >
> >  /* Must immediately follow do_nothing for size calculuations to work out. */
> > -static void do_overwritten(void)
> > +static noinline void do_overwritten(void)
> >  {
> >         pr_info("do_overwritten wasn't overwritten!\n");
> >         return;
> > @@ -110,15 +110,14 @@ void lkdtm_WRITE_RO_AFTER_INIT(void)
> >
> >  void lkdtm_WRITE_KERN(void)
> >  {
> > -       size_t size;
> > -       volatile unsigned char *ptr;
> > +       unsigned long value_dow = (unsigned long)do_overwritten;
> > +       unsigned long value_do =  (unsigned long)do_nothing;
> > +       size_t size = (size_t)(value_dow > value_do ?
> > +                       value_dow - value_do : value_do - value_dow);
> >
> > -       size = (unsigned long)do_overwritten - (unsigned long)do_nothing;
>
> Thanks for the patch! Might it be simpler to do:
>
> size = abs((uintptr_t)do_overwritten - (uintptr_t)do_nothing));
>
> Then I don't think much of this function needs to be changed.
>

Thanks Nick.

abs() is better, I will check the patch.

Regards,
Candle


> > -       ptr = (unsigned char *)do_overwritten;
> > -
> > -       pr_info("attempting bad %zu byte write at %px\n", size, ptr);
> > -       memcpy((void *)ptr, (unsigned char *)do_nothing, size);
> > -       flush_icache_range((unsigned long)ptr, (unsigned long)(ptr + size));
> > +       pr_info("attempting bad %zu byte write at %px\n", size, do_overwritten);
> > +       memcpy((void *)value_dow, (void *)value_do, size);
> > +       flush_icache_range(value_dow, value_dow + (unsigned long)size);
> >         pr_err("FAIL: survived bad write\n");
> >
> >         do_overwritten();
> > --
> > 2.17.0
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers

Re: [PATCH] lkdtm: fix memory copy size for WRITE_KERN

[Nick Desaulniers]

On Tue, Jan 26, 2021 at 6:13 AM Candle Sun <candlesea@gmail.com> wrote:
>
> On Mon, Jan 25, 2021 at 6:37 PM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Candle Sun
> > > Sent: 25 January 2021 08:56
> > >
> > > From: Candle Sun <candle.sun@unisoc.com>
> > >
> > > Though do_overwritten() follows do_nothing() in source code, the final
> > > memory address order is determined by compiler. We can't always assume
> > > address of do_overwritten() is bigger than do_nothing(). At least the
> > > Clang we are using places do_overwritten() before do_nothing() in the
> > > object. This causes the copy size in lkdtm_WRITE_KERN() is *really*
> > > big and WRITE_KERN test on ARM32 arch will fail.
> > >
> > > Compare the address order before doing the subtraction.
> >
> > It isn't clear that helps.
> > Compile with -ffunction-sections and/or do LTO an there
> > is no reason at all why the functions should be together.
> >
> > Even without that lkdtm_WRITE_KERN() could easily be between them.
> >
> > You need to get the size of the 'empty function' from the
> > symbol table.
> >
> >         David
>
> Thanks David.
>
> I think using abs() by Nick's advice would be better. But could you
> point out which kernel function can get function size?

The Elf symbol table should contain this info, IIUC.

Given a string literal of a symbol (such as a function identifier),
kallsyms_lookup_name() can be used to return its address.

From there we'd want to fetch the Elf_Sym for the address which should
contain a st_size field which I think corresponds to the size in bytes
of the function.  (At least, from playing with `llvm-readelf -s`)
Probably would want to validate it's an STT_FUNC symbol type, too.  We
basically want something like kexec_purgatory_find_symbol(), but that
knows about the entire kernel image, and not the purgatory image used
during kexec.  I don't see any such function currently in the
kernel...but it's a large codebase to search through.

>
> Regards,
> Candle
>
>
> >
> > >
> > > Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> > > ---
> > >  drivers/misc/lkdtm/perms.c | 19 +++++++++----------
> > >  1 file changed, 9 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> > > index 2dede2ef658f..fbfbdf89d668 100644
> > > --- a/drivers/misc/lkdtm/perms.c
> > > +++ b/drivers/misc/lkdtm/perms.c
> > > @@ -31,13 +31,13 @@ static unsigned long ro_after_init __ro_after_init = 0x55AA5500;
> > >   * This just returns to the caller. It is designed to be copied into
> > >   * non-executable memory regions.
> > >   */
> > > -static void do_nothing(void)
> > > +static noinline void do_nothing(void)
> > >  {
> > >       return;
> > >  }
> > >
> > >  /* Must immediately follow do_nothing for size calculuations to work out. */
> > > -static void do_overwritten(void)
> > > +static noinline void do_overwritten(void)
> > >  {
> > >       pr_info("do_overwritten wasn't overwritten!\n");
> > >       return;
> > > @@ -110,15 +110,14 @@ void lkdtm_WRITE_RO_AFTER_INIT(void)
> > >
> > >  void lkdtm_WRITE_KERN(void)
> > >  {
> > > -     size_t size;
> > > -     volatile unsigned char *ptr;
> > > +     unsigned long value_dow = (unsigned long)do_overwritten;
> > > +     unsigned long value_do =  (unsigned long)do_nothing;
> > > +     size_t size = (size_t)(value_dow > value_do ?
> > > +                     value_dow - value_do : value_do - value_dow);
> > >
> > > -     size = (unsigned long)do_overwritten - (unsigned long)do_nothing;
> > > -     ptr = (unsigned char *)do_overwritten;
> > > -
> > > -     pr_info("attempting bad %zu byte write at %px\n", size, ptr);
> > > -     memcpy((void *)ptr, (unsigned char *)do_nothing, size);
> > > -     flush_icache_range((unsigned long)ptr, (unsigned long)(ptr + size));
> > > +     pr_info("attempting bad %zu byte write at %px\n", size, do_overwritten);
> > > +     memcpy((void *)value_dow, (void *)value_do, size);
> > > +     flush_icache_range(value_dow, value_dow + (unsigned long)size);
> > >       pr_err("FAIL: survived bad write\n");
> > >
> > >       do_overwritten();
> > > --
> > > 2.17.0
> >
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > Registration No: 1397386 (Wales)
> >



-- 
Thanks,
~Nick Desaulniers

RE: [PATCH] lkdtm: fix memory copy size for WRITE_KERN

[David Laight]

From: Nick Desaulniers
> Sent: 26 January 2021 18:40
> 
> On Tue, Jan 26, 2021 at 6:13 AM Candle Sun <candlesea@gmail.com> wrote:
> >
> > On Mon, Jan 25, 2021 at 6:37 PM David Laight <David.Laight@aculab.com> wrote:
> > >
> > > From: Candle Sun
> > > > Sent: 25 January 2021 08:56
> > > >
> > > > From: Candle Sun <candle.sun@unisoc.com>
> > > >
> > > > Though do_overwritten() follows do_nothing() in source code, the final
> > > > memory address order is determined by compiler. We can't always assume
> > > > address of do_overwritten() is bigger than do_nothing(). At least the
> > > > Clang we are using places do_overwritten() before do_nothing() in the
> > > > object. This causes the copy size in lkdtm_WRITE_KERN() is *really*
> > > > big and WRITE_KERN test on ARM32 arch will fail.
> > > >
> > > > Compare the address order before doing the subtraction.
> > >
> > > It isn't clear that helps.
> > > Compile with -ffunction-sections and/or do LTO an there
> > > is no reason at all why the functions should be together.
> > >
> > > Even without that lkdtm_WRITE_KERN() could easily be between them.
> > >
> > > You need to get the size of the 'empty function' from the
> > > symbol table.
> > >
> > >         David
> >
> > Thanks David.
> >
> > I think using abs() by Nick's advice would be better. But could you
> > point out which kernel function can get function size?
> 
> The Elf symbol table should contain this info, IIUC.
> 
> Given a string literal of a symbol (such as a function identifier),
> kallsyms_lookup_name() can be used to return its address.
> 
> From there we'd want to fetch the Elf_Sym for the address which should
> contain a st_size field which I think corresponds to the size in bytes
> of the function.  (At least, from playing with `llvm-readelf -s`)
> Probably would want to validate it's an STT_FUNC symbol type, too.  We
> basically want something like kexec_purgatory_find_symbol(), but that
> knows about the entire kernel image, and not the purgatory image used
> during kexec.  I don't see any such function currently in the
> kernel...but it's a large codebase to search through.

The alternative is to get the linker script to define a specific
constant to the size of the function.
You can then link against it (by using it as the address of a symbol).

It may be easier to use an asm file for the 'return 0' code.
I'm guessing there are things in the static branch area
that could be used.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] lkdtm: fix memory copy size for WRITE_KERN

[Candle Sun]

Thanks  David and Nick.

For simpleness. I just use abs() to get the copy size.

Patch version 2 is mailed. Please help review it.

Regards,
Candle


On Wed, Jan 27, 2021 at 6:53 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Nick Desaulniers
> > Sent: 26 January 2021 18:40
> >
> > On Tue, Jan 26, 2021 at 6:13 AM Candle Sun <candlesea@gmail.com> wrote:
> > >
> > > On Mon, Jan 25, 2021 at 6:37 PM David Laight <David.Laight@aculab.com> wrote:
> > > >
> > > > From: Candle Sun
> > > > > Sent: 25 January 2021 08:56
> > > > >
> > > > > From: Candle Sun <candle.sun@unisoc.com>
> > > > >
> > > > > Though do_overwritten() follows do_nothing() in source code, the final
> > > > > memory address order is determined by compiler. We can't always assume
> > > > > address of do_overwritten() is bigger than do_nothing(). At least the
> > > > > Clang we are using places do_overwritten() before do_nothing() in the
> > > > > object. This causes the copy size in lkdtm_WRITE_KERN() is *really*
> > > > > big and WRITE_KERN test on ARM32 arch will fail.
> > > > >
> > > > > Compare the address order before doing the subtraction.
> > > >
> > > > It isn't clear that helps.
> > > > Compile with -ffunction-sections and/or do LTO an there
> > > > is no reason at all why the functions should be together.
> > > >
> > > > Even without that lkdtm_WRITE_KERN() could easily be between them.
> > > >
> > > > You need to get the size of the 'empty function' from the
> > > > symbol table.
> > > >
> > > >         David
> > >
> > > Thanks David.
> > >
> > > I think using abs() by Nick's advice would be better. But could you
> > > point out which kernel function can get function size?
> >
> > The Elf symbol table should contain this info, IIUC.
> >
> > Given a string literal of a symbol (such as a function identifier),
> > kallsyms_lookup_name() can be used to return its address.
> >
> > From there we'd want to fetch the Elf_Sym for the address which should
> > contain a st_size field which I think corresponds to the size in bytes
> > of the function.  (At least, from playing with `llvm-readelf -s`)
> > Probably would want to validate it's an STT_FUNC symbol type, too.  We
> > basically want something like kexec_purgatory_find_symbol(), but that
> > knows about the entire kernel image, and not the purgatory image used
> > during kexec.  I don't see any such function currently in the
> > kernel...but it's a large codebase to search through.
>
> The alternative is to get the linker script to define a specific
> constant to the size of the function.
> You can then link against it (by using it as the address of a symbol).
>
> It may be easier to use an asm file for the 'return 0' code.
> I'm guessing there are things in the static branch area
> that could be used.
>
>         David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

Re: [PATCH] lkdtm: fix memory copy size for WRITE_KERN

[Nick Desaulniers]

On Tue, Jan 26, 2021 at 2:53 PM David Laight <David.Laight@aculab.com> wrote:
>
> From: Nick Desaulniers
> > Sent: 26 January 2021 18:40
> >
> > On Tue, Jan 26, 2021 at 6:13 AM Candle Sun <candlesea@gmail.com> wrote:
> > >
> > > On Mon, Jan 25, 2021 at 6:37 PM David Laight <David.Laight@aculab.com> wrote:
> > > >
> > > > From: Candle Sun
> > > > > Sent: 25 January 2021 08:56
> > > > >
> > > > > From: Candle Sun <candle.sun@unisoc.com>
> > > > >
> > > > > Though do_overwritten() follows do_nothing() in source code, the final
> > > > > memory address order is determined by compiler. We can't always assume
> > > > > address of do_overwritten() is bigger than do_nothing(). At least the
> > > > > Clang we are using places do_overwritten() before do_nothing() in the
> > > > > object. This causes the copy size in lkdtm_WRITE_KERN() is *really*
> > > > > big and WRITE_KERN test on ARM32 arch will fail.
> > > > >
> > > > > Compare the address order before doing the subtraction.
> > > >
> > > > It isn't clear that helps.
> > > > Compile with -ffunction-sections and/or do LTO an there
> > > > is no reason at all why the functions should be together.
> > > >
> > > > Even without that lkdtm_WRITE_KERN() could easily be between them.
> > > >
> > > > You need to get the size of the 'empty function' from the
> > > > symbol table.
> > > >
> > > >         David
> > >
> > > Thanks David.
> > >
> > > I think using abs() by Nick's advice would be better. But could you
> > > point out which kernel function can get function size?
> >
> > The Elf symbol table should contain this info, IIUC.
> >
> > Given a string literal of a symbol (such as a function identifier),
> > kallsyms_lookup_name() can be used to return its address.
> >
> > From there we'd want to fetch the Elf_Sym for the address which should
> > contain a st_size field which I think corresponds to the size in bytes
> > of the function.  (At least, from playing with `llvm-readelf -s`)
> > Probably would want to validate it's an STT_FUNC symbol type, too.  We
> > basically want something like kexec_purgatory_find_symbol(), but that
> > knows about the entire kernel image, and not the purgatory image used
> > during kexec.  I don't see any such function currently in the
> > kernel...but it's a large codebase to search through.
>
> The alternative is to get the linker script to define a specific
> constant to the size of the function.
> You can then link against it (by using it as the address of a symbol).

Or use __attribute__((__section())) to place the code we need to
measure in a custom section, then in the linker script define symbols
that bound the section then place it in .text.  That pattern is used
throughout the kernel.  But I suspect this is overkill for this
module.

A constant would depend on the arch.

>
> It may be easier to use an asm file for the 'return 0' code.
> I'm guessing there are things in the static branch area
> that could be used.
>
>         David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)



-- 
Thanks,
~Nick Desaulniers