Date:   Thu, 16 Dec 2021 11:08:26 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     Kees Cook <keescook@...omium.org>
Cc:     linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 06/17] fortify: Detect struct member overflows in
 memcpy() at compile-time

On Mon, Dec 13, 2021 at 02:33:20PM -0800, Kees Cook wrote:
> memcpy() is dead; long live memcpy()
> 
> tl;dr: In order to eliminate a large class of common buffer overflow
> flaws that continue to persist in the kernel, have memcpy() (under
> CONFIG_FORTIFY_SOURCE) perform bounds checking of the destination struct
> member when they have a known size. This would have caught all of the
> memcpy()-related buffer write overflow flaws identified in at least the
> last three years.
> 

Hi Kees,

Since there's a *lot* of context below, it's very easy to miss some key details
(e.g. that the compile-time warnings are limited to W=1 builds). It would be
really nice if the summary above could say something like:

  This patch makes it possible to detect when memcpy() of a struct member may
  go past the bounds of that member. When CONFIG_FORTIFY_SOURCE=y, runtime
  checks are always emitted where the compiler cannot guarantee a memcpy() is
  safely bounded, and compile-time warnings are enabled for W=1 builds.

  This catches a large class of common buffer overflow flaws, and would have
  caught all of the memcpy()-related buffer write overflow flaws identified in
  the last three years.

As an aside, since W=1 is chock-full of (IMO useless) warnings, is there any
way to enable *just* the FORTIFY_SOURCE warnings?

[...]

> Implementation:
> 
> Tighten the memcpy() destination buffer size checking to use the actual
> ("mode 1") target buffer size as the bounds check instead of their
> enclosing structure's ("mode 0") size. Use a common inline for memcpy()
> (and memmove() in a following patch), since all the tests are the
> same. All new cross-field memcpy() uses must use the struct_group() macro
> or similar to target a specific range of fields, so that FORTIFY_SOURCE
> can reason about the size and safety of the copy.
> 
> For now, cross-member "mode 1" read detection at compile-time will be
> limited to W=1 builds, since it is, unfortunately, very common. As the
> priority is solving write overflows, read overflows can be part of the
> next phase.

I had a go at testing this on arm64, and could get build-time warnings from GCC
11.1.0, but not from Clang 13.0.0.

I picked the series from:

  https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=memcpy/step2/next-20211213

I prepped a tree with that branch and a deliberate bug:

| [mark@...rids:~/src/linux]% git checkout -f kernel-org-kees/memcpy/step2/next-20211213
| [mark@...rids:~/src/linux]% git clean -qfdx
| [mark@...rids:~/src/linux]% cat <<EOF >> arch/arm64/kernel/setup.c
| 
| struct foo {
|        int a;
|        int b;
| } foo1, foo2;
| 
| void foo_copy(void);
| void foo_copy(void)
| {
|        memcpy(&foo1.a, &foo2.a, sizeof(foo1));
| }
| EOF

When building with GCC 11.1.0:

| [mark@...rids:~/src/linux]% usekorg 11.1.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- -s defconfig
| [mark@...rids:~/src/linux]% ./scripts/config -e FORTIFY_SOURCE                                      
| [mark@...rids:~/src/linux]% grep FORTIFY_SOURCE .config       
| CONFIG_ARCH_HAS_FORTIFY_SOURCE=y
| CONFIG_FORTIFY_SOURCE=y
| [mark@...rids:~/src/linux]% usekorg 11.1.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- -s arch/arm64/kernel/setup.o
| In file included from ./include/linux/string.h:253,
|                  from ./include/linux/bitmap.h:11,
|                  from ./include/linux/cpumask.h:12,
|                  from ./include/linux/smp.h:13,
|                  from ./include/linux/lockdep.h:14,
|                  from ./include/linux/mutex.h:17,
|                  from ./include/linux/kernfs.h:12,
|                  from ./include/linux/sysfs.h:16,
|                  from ./include/linux/kobject.h:20,
|                  from ./include/linux/of.h:17,
|                  from ./include/linux/irqdomain.h:35,
|                  from ./include/linux/acpi.h:13,
|                  from arch/arm64/kernel/setup.c:9:
| In function 'fortify_memcpy_chk',
|     inlined from 'foo_copy' at arch/arm64/kernel/setup.c:457:8:
| ./include/linux/fortify-string.h:316:25: warning: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
|   316 |                         __write_overflow_field(p_size_field, size);
|       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| ./include/linux/fortify-string.h:324:25: warning: call to '__read_overflow2_field' declared with attribute warning: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
|   324 |                         __read_overflow2_field(q_size_field, size);
|       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

When building with clang 13.0.0 (the vdso bits can be ignored):

| [mark@...rids:~/src/linux]% usellvm 13.0.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- LLVM=1 -s defconfig                    
| [mark@...rids:~/src/linux]% ./scripts/config -e FORTIFY_SOURCE                                             
| [mark@...rids:~/src/linux]% grep FORTIFY_SOURCE .config       
| CONFIG_ARCH_HAS_FORTIFY_SOURCE=y
| CONFIG_FORTIFY_SOURCE=y
| [mark@...rids:~/src/linux]% usellvm 13.0.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- LLVM=1 W=1 -s arch/arm64/kernel/setup.o
| arch/arm64/kernel/vdso/vgettimeofday.c:9:5: warning: no previous prototype for function '__kernel_clock_gettime' [-Wmissing-prototypes]
| int __kernel_clock_gettime(clockid_t clock,
|     ^
| arch/arm64/kernel/vdso/vgettimeofday.c:9:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
| int __kernel_clock_gettime(clockid_t clock,
| ^
| static 
| arch/arm64/kernel/vdso/vgettimeofday.c:15:5: warning: no previous prototype for function '__kernel_gettimeofday' [-Wmissing-prototypes]
| int __kernel_gettimeofday(struct __kernel_old_timeval *tv,
|     ^
| arch/arm64/kernel/vdso/vgettimeofday.c:15:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
| int __kernel_gettimeofday(struct __kernel_old_timeval *tv,
| ^
| static 
| arch/arm64/kernel/vdso/vgettimeofday.c:21:5: warning: no previous prototype for function '__kernel_clock_getres' [-Wmissing-prototypes]
| int __kernel_clock_getres(clockid_t clock_id,
|     ^
| arch/arm64/kernel/vdso/vgettimeofday.c:21:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
| int __kernel_clock_getres(clockid_t clock_id,
| ^
| static 
| 3 warnings generated.

No relevant warnings, but code was generated for runtime warnings:

| 0000000000000000 <foo_copy>:
|    0:   d503233f        paciasp
|    4:   a9bf7bfd        stp     x29, x30, [sp, #-16]!
|    8:   910003fd        mov     x29, sp
|    c:   52800080        mov     w0, #0x4                        // #4
|   10:   52800101        mov     w1, #0x8                        // #8
|   14:   94000000        bl      0 <__write_overflow_field>
|   18:   52800080        mov     w0, #0x4                        // #4
|   1c:   52800101        mov     w1, #0x8                        // #8
|   20:   94000000        bl      0 <__read_overflow2_field>
|   24:   90000008        adrp    x8, 8 <foo_copy+0x8>
|   28:   f9400108        ldr     x8, [x8]
|   2c:   90000009        adrp    x9, 0 <foo_copy>
|   30:   f9000128        str     x8, [x9]
|   34:   a8c17bfd        ldp     x29, x30, [sp], #16
|   38:   d50323bf        autiasp
|   3c:   d65f03c0        ret

Have I misunderstood how that's meant to work, or am I doing something wrong?

Thanks,
Mark.

Powered by blists - more mailing lists