* [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 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-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
* 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 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
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.