From: Mark Rutland <mark.rutland@arm.com>
To: Kees Cook <keescook@chromium.org>
Cc: linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 06/17] fortify: Detect struct member overflows in memcpy() at compile-time
Date: Thu, 16 Dec 2021 11:08:26 +0000 [thread overview]
Message-ID: <YbseKuBwHEfvzykO@FVFF77S0Q05N> (raw)
In-Reply-To: <20211213223331.135412-7-keescook@chromium.org>
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@lakrids:~/src/linux]% git checkout -f kernel-org-kees/memcpy/step2/next-20211213
| [mark@lakrids:~/src/linux]% git clean -qfdx
| [mark@lakrids:~/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@lakrids:~/src/linux]% usekorg 11.1.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- -s defconfig
| [mark@lakrids:~/src/linux]% ./scripts/config -e FORTIFY_SOURCE
| [mark@lakrids:~/src/linux]% grep FORTIFY_SOURCE .config
| CONFIG_ARCH_HAS_FORTIFY_SOURCE=y
| CONFIG_FORTIFY_SOURCE=y
| [mark@lakrids:~/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@lakrids:~/src/linux]% usellvm 13.0.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- LLVM=1 -s defconfig
| [mark@lakrids:~/src/linux]% ./scripts/config -e FORTIFY_SOURCE
| [mark@lakrids:~/src/linux]% grep FORTIFY_SOURCE .config
| CONFIG_ARCH_HAS_FORTIFY_SOURCE=y
| CONFIG_FORTIFY_SOURCE=y
| [mark@lakrids:~/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.
next prev parent reply other threads:[~2021-12-16 11:08 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-13 22:33 [PATCH 00/17] Enable strict compile-time memcpy() fortify checks Kees Cook
2021-12-13 22:33 ` [PATCH 01/17] KVM: x86: Replace memset() "optimization" with normal per-field writes Kees Cook
2021-12-13 22:33 ` [PATCH 02/17] net/mlx5e: Avoid field-overflowing memcpy() Kees Cook
2021-12-13 22:33 ` [PATCH 03/17] net/mlx5e: Use struct_group() for memcpy() region Kees Cook
2021-12-13 22:33 ` [PATCH 04/17] media: omap3isp: " Kees Cook
2021-12-13 22:33 ` [PATCH 05/17] sata_fsl: " Kees Cook
2021-12-13 22:33 ` [PATCH 06/17] fortify: Detect struct member overflows in memcpy() at compile-time Kees Cook
2021-12-16 11:08 ` Mark Rutland [this message]
2021-12-16 11:21 ` Mark Rutland
2021-12-16 18:00 ` Kees Cook
2021-12-17 13:34 ` Mark Rutland
2021-12-13 22:33 ` [PATCH 07/17] fortify: Detect struct member overflows in memmove() " Kees Cook
2021-12-13 22:33 ` [PATCH 08/17] ath11k: Use memset_startat() for clearing queue descriptors Kees Cook
2021-12-14 6:02 ` Kalle Valo
2021-12-14 15:46 ` Kalle Valo
2021-12-14 17:05 ` Kees Cook
2021-12-16 13:50 ` Kalle Valo
2021-12-13 22:33 ` [PATCH 09/17] RDMA/mlx5: Use memset_after() to zero struct mlx5_ib_mr Kees Cook
2021-12-13 22:33 ` [PATCH 10/17] drbd: Use struct_group() to zero algs Kees Cook
2021-12-13 22:33 ` [PATCH 11/17] dm integrity: Use struct_group() to zero struct journal_sector Kees Cook
2021-12-13 22:33 ` [PATCH 12/17] iw_cxgb4: Use memset_startat() for cpl_t5_pass_accept_rpl Kees Cook
2021-12-13 22:33 ` [PATCH 13/17] intel_th: msu: Use memset_startat() for clearing hw header Kees Cook
2021-12-13 22:33 ` [PATCH 14/17] IB/mthca: Use memset_startat() for clearing mpt_entry Kees Cook
2021-12-13 22:33 ` [PATCH 15/17] scsi: lpfc: Use struct_group() to initialize struct lpfc_cgn_info Kees Cook
2021-12-13 22:33 ` [PATCH 16/17] fortify: Detect struct member overflows in memset() at compile-time Kees Cook
2021-12-13 22:33 ` [PATCH 17/17] fortify: Work around Clang inlining bugs Kees Cook
2021-12-15 0:26 ` [PATCH 00/17] Enable strict compile-time memcpy() fortify checks Jason Gunthorpe
2021-12-17 4:04 ` Martin K. Petersen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YbseKuBwHEfvzykO@FVFF77S0Q05N \
--to=mark.rutland@arm.com \
--cc=keescook@chromium.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).