* [PATCH] lkdtm: fix memory copy size for WRITE_KERN
@ 2021-01-25 8:56 Candle Sun
2021-01-25 10:37 ` David Laight
2021-01-25 21:16 ` Nick Desaulniers
0 siblings, 2 replies; 9+ messages in thread
From: Candle Sun @ 2021-01-25 8:56 UTC (permalink / raw)
To: keescook
Cc: arnd, gregkh, natechancellor, ndesaulniers, candle.sun,
linux-kernel, clang-built-linux
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH] lkdtm: fix memory copy size for WRITE_KERN
2021-01-25 8:56 [PATCH] lkdtm: fix memory copy size for WRITE_KERN Candle Sun
@ 2021-01-25 10:37 ` David Laight
2021-01-26 14:13 ` Candle Sun
2021-01-25 21:16 ` Nick Desaulniers
1 sibling, 1 reply; 9+ messages in thread
From: David Laight @ 2021-01-25 10:37 UTC (permalink / raw)
To: 'Candle Sun', keescook
Cc: arnd, gregkh, natechancellor, ndesaulniers, candle.sun,
linux-kernel, clang-built-linux
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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lkdtm: fix memory copy size for WRITE_KERN
2021-01-25 8:56 [PATCH] lkdtm: fix memory copy size for WRITE_KERN Candle Sun
2021-01-25 10:37 ` David Laight
@ 2021-01-25 21:16 ` Nick Desaulniers
2021-01-26 14:17 ` Candle Sun
1 sibling, 1 reply; 9+ messages in thread
From: Nick Desaulniers @ 2021-01-25 21:16 UTC (permalink / raw)
To: Candle Sun
Cc: Kees Cook, Arnd Bergmann, Greg KH, Nathan Chancellor, candle.sun,
LKML, clang-built-linux
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lkdtm: fix memory copy size for WRITE_KERN
2021-01-25 10:37 ` David Laight
@ 2021-01-26 14:13 ` Candle Sun
2021-01-26 18:39 ` Nick Desaulniers
0 siblings, 1 reply; 9+ messages in thread
From: Candle Sun @ 2021-01-26 14:13 UTC (permalink / raw)
To: David Laight
Cc: keescook, arnd, gregkh, natechancellor, ndesaulniers, candle.sun,
linux-kernel, clang-built-linux
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)
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lkdtm: fix memory copy size for WRITE_KERN
2021-01-25 21:16 ` Nick Desaulniers
@ 2021-01-26 14:17 ` Candle Sun
0 siblings, 0 replies; 9+ messages in thread
From: Candle Sun @ 2021-01-26 14:17 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Kees Cook, Arnd Bergmann, Greg KH, Nathan Chancellor, Candle Sun,
LKML, clang-built-linux
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lkdtm: fix memory copy size for WRITE_KERN
2021-01-26 14:13 ` Candle Sun
@ 2021-01-26 18:39 ` Nick Desaulniers
2021-01-26 22:53 ` David Laight
0 siblings, 1 reply; 9+ messages in thread
From: Nick Desaulniers @ 2021-01-26 18:39 UTC (permalink / raw)
To: Candle Sun
Cc: David Laight, keescook, arnd, gregkh, natechancellor, candle.sun,
linux-kernel, clang-built-linux
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] lkdtm: fix memory copy size for WRITE_KERN
2021-01-26 18:39 ` Nick Desaulniers
@ 2021-01-26 22:53 ` David Laight
2021-01-27 11:00 ` Candle Sun
2021-01-27 17:42 ` Nick Desaulniers
0 siblings, 2 replies; 9+ messages in thread
From: David Laight @ 2021-01-26 22:53 UTC (permalink / raw)
To: 'Nick Desaulniers', Candle Sun
Cc: keescook, arnd, gregkh, natechancellor, candle.sun, linux-kernel,
clang-built-linux
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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lkdtm: fix memory copy size for WRITE_KERN
2021-01-26 22:53 ` David Laight
@ 2021-01-27 11:00 ` Candle Sun
2021-01-27 17:42 ` Nick Desaulniers
1 sibling, 0 replies; 9+ messages in thread
From: Candle Sun @ 2021-01-27 11:00 UTC (permalink / raw)
To: David Laight
Cc: Nick Desaulniers, keescook, arnd, gregkh, natechancellor,
candle.sun, linux-kernel, clang-built-linux
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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lkdtm: fix memory copy size for WRITE_KERN
2021-01-26 22:53 ` David Laight
2021-01-27 11:00 ` Candle Sun
@ 2021-01-27 17:42 ` Nick Desaulniers
1 sibling, 0 replies; 9+ messages in thread
From: Nick Desaulniers @ 2021-01-27 17:42 UTC (permalink / raw)
To: David Laight
Cc: Candle Sun, keescook, arnd, gregkh, natechancellor, candle.sun,
linux-kernel, clang-built-linux
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-01-27 17:45 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-25 8:56 [PATCH] lkdtm: fix memory copy size for WRITE_KERN Candle Sun
2021-01-25 10:37 ` David Laight
2021-01-26 14:13 ` Candle Sun
2021-01-26 18:39 ` Nick Desaulniers
2021-01-26 22:53 ` David Laight
2021-01-27 11:00 ` Candle Sun
2021-01-27 17:42 ` Nick Desaulniers
2021-01-25 21:16 ` Nick Desaulniers
2021-01-26 14:17 ` Candle Sun
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.