Re: [PATCH 0/6] RFC: adding support to GCC for detecting trust boundaries
flat view
From: Martin Sebor <hidden>
Date: 2021-12-06 18:12:03
On 11/13/21 1:37 PM, David Malcolm via Gcc-patches wrote:
[Crossposting between gcc-patches@gcc.gnu.org and linux-toolchains@vger.kernel.org; sorry about my lack of kernel knowledge, in case of the following seems bogus] I've been trying to turn my prototype from the LPC2021 session on "Adding kernel-specific test coverage to GCC's -fanalyzer option" ( https://linuxplumbersconf.org/event/11/contributions/1076/ ) into something that can go into GCC upstream without adding kernel-specific special cases, or requiring a GCC plugin. The prototype simply specialcased "copy_from_user" and "copy_to_user" in GCC, which is clearly not OK. This GCC patch kit implements detection of "trust boundaries", aimed at detection of "infoleaks" and of use of unsanitized attacker-controlled values ("taint") in the Linux kernel. For example, here's an infoleak diagnostic (using notes to express what fields and padding within a struct have not been initialized): infoleak-CVE-2011-1078-2.c: In function ‘test_1’: infoleak-CVE-2011-1078-2.c:28:9: warning: potential exposure of sensitive information by copying uninitialized data from stack across trust boundary [CWE-200] [-Wanalyzer-exposure-through-uninit-copy] 28 | copy_to_user(optval, &cinfo, sizeof(cinfo)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ‘test_1’: events 1-3 | | 21 | struct sco_conninfo cinfo; | | ^~~~~ | | | | | (1) region created on stack here | | (2) capacity: 6 bytes |...... | 28 | copy_to_user(optval, &cinfo, sizeof(cinfo)); | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (3) uninitialized data copied from stack here | infoleak-CVE-2011-1078-2.c:28:9: note: 1 byte is uninitialized 28 | copy_to_user(optval, &cinfo, sizeof(cinfo)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ infoleak-CVE-2011-1078-2.c:14:15: note: padding after field ‘dev_class’ is uninitialized (1 byte) 14 | __u8 dev_class[3]; | ^~~~~~~~~ infoleak-CVE-2011-1078-2.c:21:29: note: suggest forcing zero-initialization by providing a ‘{0}’ initializer 21 | struct sco_conninfo cinfo; | ^~~~~ | = {0} I have to come up with a way of expressing trust boundaries in a way that will be: - acceptable to the GCC community (not be too kernel-specific), and - useful to the Linux kernel community. At LPC it was pointed out that the kernel already has various annotations e.g. "__user" for different kinds of pointers, and that it would be best to reuse those. Approach 1: Custom Address Spaces ================================= GCC's C frontend supports target-specific address spaces; see: https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html Quoting the N1275 draft of ISO/IEC DTR 18037: "Address space names are ordinary identifiers, sharing the same name space as variables and typedef names. Any such names follow the same rules for scope as other ordinary identifiers (such as typedef names). An implementation may provide an implementation-defined set of intrinsic address spaces that are, in effect, predefined at the start of every translation unit. The names of intrinsic address spaces must be reserved identifiers (beginning with an underscore and an uppercase letter or with two underscores). An implementation may also optionally support a means for new address space names to be defined within a translation unit." Patch 1a in the following patch kit for GCC implements such a means to define new address spaces names in a translation unit, via a pragma: #prgama GCC custom_address_space(NAME_OF_ADDRESS_SPACE) For example, the Linux kernel could perhaps write: #define __kernel #pragma GCC custom_address_space(__user) #pragma GCC custom_address_space(__iomem) #pragma GCC custom_address_space(__percpu) #pragma GCC custom_address_space(__rcu) and thus the C frontend can complain about code that mismatches __user and kernel pointers, e.g.: custom-address-space-1.c: In function ‘test_argpass_to_p’: custom-address-space-1.c:29:14: error: passing argument 1 of ‘accepts_p’ from pointer to non-enclosed address space 29 | accepts_p (p_user); | ^~~~~~ custom-address-space-1.c:21:24: note: expected ‘void *’ but argument is of type ‘__user void *’ 21 | extern void accepts_p (void *); | ^~~~~~ custom-address-space-1.c: In function ‘test_cast_k_to_u’: custom-address-space-1.c:135:12: warning: cast to ‘__user’ address space pointer from disjoint generic address space pointer 135 | p_user = (void __user *)p_kernel; | ^
This seems like an excellent use of named address spaces :) I'm familiar with TR 18037 but I'm not an expert on this stuff so I can't really say a whole lot more. My only suggestion here is to follow the terminology from there in the naming of the pragma, unless you have some reason not to. I'd also recommend to consider other implementations of named address spaces, if there are any, especially those that try to be compatible with GCC. If there are none, rather than custom_address_space I'd suggest either just address_space or named_address_space. I have not yet looked at the implementation so this is just a high-level comment on the design.
The patch doesn't yet maintain a good distinction between implicit
target-specific address spaces and user-defined address spaces, has at
least one known major bug, and has only been lightly tested. I can
fix these issues, but was hoping for feedback that this approach is the
right direction from both the GCC and Linux development communities.
Implementation status: doesn't yet bootstrap; am running into stage2
vs stage3 comparison issues.
Approach 2: An "untrusted" attribute
====================================
Alternatively, patch 1b in the kit implements:
__attribute__((untrusted))
which can be applied to types as a qualifier (similarly to const,
volatile, etc) to mark a trust boundary, hence the kernel could have:
#define __user __attribute__((untrusted))
where my patched GCC treats
T *
vs
T __attribute__((untrusted)) *
as being different types and thus the C frontend can complain (even without
-fanalyzer) about e.g.:
extern void accepts_p(void *);
void test_argpass_to_p(void __user *p_user)
{
accepts_p(p_user);
}
untrusted-pointer-1.c: In function ‘test_argpass_to_p’:
untrusted-pointer-1.c:22:13: error: passing argument 1 of ‘accepts_p’
from pointer with different trust level
22 | accepts_p(p_user);
| ^~~~~~
untrusted-pointer-1.c:14:23: note: expected ‘void *’ but argument is of
type ‘__attribute__((untrusted)) void *’
14 | extern void accepts_p(void *);
| ^~~~~~
So you'd get enforcement of __user vs non-__user pointers as part of
GCC's regular type-checking. (You need an explicit cast to convert
between the untrusted vs trusted types).As with the named address space idea, this approach also looks reasonable to me. If you anticipate using the attribute only in the analyzer I would suggest to consider introducing it in the analyzer's namespace (e.g., analyzer::untrusted, or even gnu::analyzer::untrusted). I'll try to loook at the patch itself sometime later this week and comment on the implementation there.
This approach is much less expressive that the custom addres space approach; it would only cover the trust boundary aspect; it wouldn't cover any differences between generic pointers and __user, vs __iomem, __percpu, and __rcu which I admit I only dimly understand. Implementation status: bootstraps and passes regression testing. Builds most of the kernel, but am running into various conversion issues. It would be good to have some clarity on what conversions the compiler ought to warn about, and what conversions should be OK. Approach 3: some kind of custom qualifier ========================================= Approach 1 extends the existing "named address space" machinery to add new values; approach 2 adds a new flag to cv-qualifiers. Both of these approaches work in terms of cv-qualifiers. We have some spare bits available for these; perhaps a third approach could be to add a new kind of user-defined qualifier, like named address spaces, but othogonal to them. I haven't attempted to implement this.
I'm afraid I don't understand what this would be useful for enough to comment.
Other attributes ================ Patch 2 in the kit adds: __attribute__((returns_zero_on_success)) and __attribute__((returns_nonzero_on_success)) as hints to the analyzer that it's worth bifurcating the analysis of such functions (to explore failure vs success, and thus to better explore error-handling paths). It's also a hint to the human reader of the source code.
I thing being able to express something along these lines would be useful even outside the analyzer, both for warnings and, when done right, perhaps also for optimization. So I'm in favor of something like this. I'll just reiterate here the comment on this attribute I sent you privately some time ago. A more general attribute would also make it possible to specify the value (or argument) on success and failure. With those we would be able to express the return values of the POSIX read and write functions and others like it: ssize_t read (int fildes, void *buf, size_t nbyte); ssize_t write (int fildes, const void *buf, size_t nbyte); I.e., it would be nice to express that the return value is also the number of bytes (elements?) of the array the function wrote into. This, along with symbolic evaluation in the middle end, would then let us detect uninitialized reads back in the function's caller (after read) and similar. This is just an idea, and there may be more general apoproaches that would be even more expressive. But it's probably too late in the development cycle to design and add those to GCC 12. As I promised, I'll try to look at the meat of each patch and give you some comments, hopefully later this week. Martin
Given the above, the kernel could then have:
extern int copy_from_user(void *to, const void __user *from, long n)
__attribute__((access (write_only, 1, 3),
access (read_only, 2, 3),
returns_zero_on_success));
extern long copy_to_user(void __user *to, const void *from, unsigned long n)
__attribute__((access (write_only, 1, 3),
access (read_only, 2, 3),
returns_zero_on_success));
with suitable macros in compiler.h or whatnot.
("access" is an existing GCC attribute; see
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html )
My patched GCC add a heuristic to -fanalyzer that a 3-argument function
with a read_only buffer, a write_only buffer and a shared size argument
is a "copy function", and treats it as a copy from *from to *to of up to
n bytes that succeeds, or, given one of the above attributes can succeed
or fail. I'm wiring things up so that values read from *untrusted_ptr
are tracked as tainted, and values written to *untrusted_ptr are treated
as possible infoleaks (e.g. uninitialized values written to
*untrusted_ptr are specifically called out). This gets the extra
checking for infoleaks and taint that my earlier prototype had, but is
thus expressed via attributes, without having to have kernel-specific
special cases.
Patch 3 of the kit adds infoleak detection to GCC's -fanalyzer (as
in the example above).
Possibly silly question: is it always a bug for the value of a kernel
pointer to leak into user space? i.e. should I be complaining about an
infoleak if the value of a trusted_ptr itself is written to
*untrusted_ptr? e.g.
s.p = some_kernel_ptr;
copy_to_user(user_p, &s, sizeof (s));
/* value of some_kernel_ptr is written to user space;
is this something we should warn for? */
Patch 4a/4b wire up the different implementations of "untrusted" into
GCC's -fanalyzer, which is used by...
Patch 5 uses this so that "untrusted" values are used in taint detection
in the analyzer, so that it can complain about attacker-controlled
values being used without sanitization.
Patch 6 adds a new __attribute__ ((tainted)) allowing for further
taint detection (e.g. identifying syscalls), with minimal patching of
the kernel, and without requiring a lot of link-time interprocedural
analysis. I believe that some of this could work independently of
the trust boundary marking from the rest of the patch kit.
The combined patch kit (using approach 2 i.e. the "b" patches)
successfully bootstraps and passes regression testing on
x86_64-pc-linux-gnu.
Which of the 3 approaches looks best to:
- the GCC community?
- the Linux kernel community?
Does clang/LLVM have anything similar?
There are many examples in the patches, some of which are taken from
historical kernel vulnerabilities, and others from my "antipatterns.ko"
project ( https://github.com/davidmalcolm/antipatterns.ko ).
Thoughts?
Dave
David Malcolm (6 or 8, depending how you count):
1a: RFC: Implement "#pragma GCC custom_address_space"
1b: Add __attribute__((untrusted))
2: Add returns_zero_on_success/failure attributes
3: analyzer: implement infoleak detection
4a: analyzer: implemention of region::untrusted_p in terms of custom
address spaces
4b: analyzer: implement region::untrusted_p in terms of
__attribute__((untrusted))
5: analyzer: use region::untrusted_p in taint detection
6: Add __attribute__ ((tainted))
gcc/Makefile.in | 3 +-
gcc/analyzer/analyzer.opt | 20 +
gcc/analyzer/checker-path.cc | 104 +++
gcc/analyzer/checker-path.h | 47 +
gcc/analyzer/diagnostic-manager.cc | 75 +-
gcc/analyzer/diagnostic-manager.h | 3 +-
gcc/analyzer/engine.cc | 342 ++++++-
gcc/analyzer/exploded-graph.h | 3 +
gcc/analyzer/pending-diagnostic.cc | 30 +
gcc/analyzer/pending-diagnostic.h | 24 +
gcc/analyzer/program-state.cc | 26 +-
gcc/analyzer/region-model-impl-calls.cc | 26 +-
gcc/analyzer/region-model.cc | 504 ++++++++++-
gcc/analyzer/region-model.h | 46 +-
gcc/analyzer/region.cc | 52 ++
gcc/analyzer/region.h | 4 +
gcc/analyzer/sm-taint.cc | 839 ++++++++++++++++--
gcc/analyzer/sm.h | 9 +
gcc/analyzer/store.h | 1 +
gcc/analyzer/trust-boundaries.cc | 615 +++++++++++++
gcc/c-family/c-attribs.c | 132 +++
gcc/c-family/c-pretty-print.c | 2 +
gcc/c/c-typeck.c | 64 ++
gcc/doc/extend.texi | 63 +-
gcc/doc/invoke.texi | 80 +-
gcc/print-tree.c | 3 +
.../c-c++-common/attr-returns-zero-on-1.c | 68 ++
gcc/testsuite/c-c++-common/attr-untrusted-1.c | 165 ++++
.../gcc.dg/analyzer/attr-tainted-1.c | 88 ++
.../gcc.dg/analyzer/attr-tainted-misuses.c | 6 +
.../gcc.dg/analyzer/copy-function-1.c | 98 ++
.../gcc.dg/analyzer/copy_from_user-1.c | 45 +
gcc/testsuite/gcc.dg/analyzer/infoleak-1.c | 181 ++++
gcc/testsuite/gcc.dg/analyzer/infoleak-2.c | 29 +
gcc/testsuite/gcc.dg/analyzer/infoleak-3.c | 141 +++
gcc/testsuite/gcc.dg/analyzer/infoleak-5.c | 35 +
.../analyzer/infoleak-CVE-2011-1078-1.c | 134 +++
.../analyzer/infoleak-CVE-2011-1078-2.c | 42 +
.../analyzer/infoleak-CVE-2014-1446-1.c | 117 +++
.../analyzer/infoleak-CVE-2017-18549-1.c | 101 +++
.../analyzer/infoleak-CVE-2017-18550-1.c | 171 ++++
.../gcc.dg/analyzer/infoleak-antipatterns-1.c | 162 ++++
.../gcc.dg/analyzer/infoleak-fixit-1.c | 22 +
gcc/testsuite/gcc.dg/analyzer/pr93382.c | 2 +-
.../analyzer/taint-CVE-2011-0521-1-fixed.c | 113 +++
.../gcc.dg/analyzer/taint-CVE-2011-0521-1.c | 113 +++
.../analyzer/taint-CVE-2011-0521-2-fixed.c | 93 ++
.../gcc.dg/analyzer/taint-CVE-2011-0521-2.c | 93 ++
.../analyzer/taint-CVE-2011-0521-3-fixed.c | 56 ++
.../gcc.dg/analyzer/taint-CVE-2011-0521-3.c | 57 ++
.../gcc.dg/analyzer/taint-CVE-2011-0521-4.c | 40 +
.../gcc.dg/analyzer/taint-CVE-2011-0521-5.c | 42 +
.../gcc.dg/analyzer/taint-CVE-2011-0521-6.c | 37 +
.../gcc.dg/analyzer/taint-CVE-2011-0521.h | 136 +++
.../gcc.dg/analyzer/taint-CVE-2011-2210-1.c | 93 ++
.../gcc.dg/analyzer/taint-CVE-2020-13143-1.c | 38 +
.../gcc.dg/analyzer/taint-CVE-2020-13143-2.c | 32 +
.../gcc.dg/analyzer/taint-CVE-2020-13143.h | 91 ++
gcc/testsuite/gcc.dg/analyzer/taint-alloc-1.c | 64 ++
gcc/testsuite/gcc.dg/analyzer/taint-alloc-2.c | 27 +
gcc/testsuite/gcc.dg/analyzer/taint-alloc-3.c | 21 +
gcc/testsuite/gcc.dg/analyzer/taint-alloc-4.c | 31 +
.../gcc.dg/analyzer/taint-antipatterns-1.c | 137 +++
.../gcc.dg/analyzer/taint-divisor-1.c | 26 +
.../{taint-1.c => taint-read-index-1.c} | 19 +-
.../gcc.dg/analyzer/taint-read-offset-1.c | 128 +++
.../taint-read-through-untrusted-ptr-1.c | 37 +
gcc/testsuite/gcc.dg/analyzer/taint-size-1.c | 32 +
.../gcc.dg/analyzer/taint-write-index-1.c | 132 +++
.../gcc.dg/analyzer/taint-write-offset-1.c | 132 +++
gcc/testsuite/gcc.dg/analyzer/test-uaccess.h | 19 +
.../torture/infoleak-net-ethtool-ioctl.c | 78 ++
.../torture/infoleak-vfio_iommu_type1.c | 39 +
gcc/tree-core.h | 6 +-
gcc/tree.c | 1 +
gcc/tree.h | 11 +-
76 files changed, 6558 insertions(+), 140 deletions(-)
create mode 100644 gcc/analyzer/trust-boundaries.cc
create mode 100644 gcc/testsuite/c-c++-common/attr-returns-zero-on-1.c
create mode 100644 gcc/testsuite/c-c++-common/attr-untrusted-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/attr-tainted-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/attr-tainted-misuses.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/copy-function-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/copy_from_user-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/infoleak-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/infoleak-2.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/infoleak-3.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/infoleak-5.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/infoleak-CVE-2011-1078-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/infoleak-CVE-2011-1078-2.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/infoleak-CVE-2014-1446-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/infoleak-CVE-2017-18549-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/infoleak-CVE-2017-18550-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/infoleak-antipatterns-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/infoleak-fixit-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-CVE-2011-0521-1-fixed.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-CVE-2011-0521-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-CVE-2011-0521-2-fixed.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-CVE-2011-0521-2.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-CVE-2011-0521-3-fixed.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-CVE-2011-0521-3.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-CVE-2011-0521-4.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-CVE-2011-0521-5.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-CVE-2011-0521-6.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-CVE-2011-0521.h
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-CVE-2011-2210-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-CVE-2020-13143-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-CVE-2020-13143-2.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-CVE-2020-13143.h
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-alloc-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-alloc-2.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-alloc-3.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-alloc-4.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-antipatterns-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-divisor-1.c
rename gcc/testsuite/gcc.dg/analyzer/{taint-1.c => taint-read-index-1.c} (72%)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-read-offset-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-read-through-untrusted-ptr-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-size-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-write-index-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-write-offset-1.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/test-uaccess.h
create mode 100644 gcc/testsuite/gcc.dg/analyzer/torture/infoleak-net-ethtool-ioctl.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/torture/infoleak-vfio_iommu_type1.c