[comment]: # (This presentation was made with markdown-slides) [comment]: # (This is a CommonMark compliant comment. It will not be included in the presentation.) [comment]: # (Compile this presentation with the command below) [comment]: # (mdslides presentation.md --include media) [comment]: # (Set the theme:) [comment]: # (The list of themes is at https://revealjs.com/themes/) [comment]: # (The list of code themes is at https://highlightjs.org/) [comment]: # (Pass optional settings to reveal.js:) [comment]: # (Other settings are documented at https://revealjs.com/config/) Linux Conf AU | January 15, 2022 ## Meaningful Bounds Checking in the Linux Kernel Kees ("Case") Cook @kees_cook keescook@chromium.org https://outflux.net/slides/2022/lca/

### 25 "buffer overflow" CVEs in last 3 years... [comment]: # (chartjs config https://www.chartjs.org/docs/3.2.0/general/fonts.html) <div style="height:500px; width:900px; margin: auto;"> <canvas id="cves" data-chart="pie"> ,array index overflow (7),memcpy overflow (11),bad allocation size (2),iovec confusion (1),type confusion (1),string handling (1) CVEs, 7, 11, 2, 1, 1, 1, <!-- { "options": { "plugins": { "legend": { "position": "right", "labels": { "boxWidth": 50, "font": { "size": 32 } } } }, "backgroundColor": [ "rgba(82,215,38,.9)", "rgba(255,0,0,.9)", "rgba(255,236,0,.9)", "rgba(255,115,0,.9)", "rgba(0,126,214,.9)", "rgba(124,221,221,.9)" ] } } --> </canvas> </div>

**7 array index overflow flaws ...** <div style="height:500px; width:900px; margin: auto;"> <canvas id="cves" data-chart="pie"> ,array index overflow (7),memcpy overflow (11),bad allocation size (2),iovec confusion (1),type confusion (1),string handling (1) CVEs, 7, 11, 2, 1, 1, 1, <!-- { "options": { "animation": false, "plugins": { "legend": { "position": "right", "labels": { "boxWidth": 50, "font": { "size": 32 } } } }, "backgroundColor": [ "rgba(82,215,38,.9)", "rgba(255,0,0,.1)", "rgba(255,236,0,.1)", "rgba(255,115,0,.1)", "rgba(0,126,214,.1)", "rgba(124,221,221,.1)" ] } } -->

Without instrumentation, there's no checking of array indexes: ```cpp[1-8] struct sockaddr_alg { ... __u8 salg_name[64]; ... } sa; int byte = 98; sa.salg_name[byte] = input; ```

All fixed-size array indexing flaws are mitigated with Undefined Behavior Sanitizer (**`-fsanitize=bounds`**) in GCC and Clang with these settings: ```shell CONFIG_UBSAN_BOUNDS=y CONFIG_UBSAN_TRAP=y or set sysctl panic_on_warn=1 ``` For example: ```shell UBSAN: array-index-out-of-bounds in crypto/af_alg.c:166:2 index 98 is out of range for type '__u8 [64]' ... Kernel panic - not syncing: panic_on_warn set ... ``` Though the `af_alg` bug replaced a fixed-size array with a flexible array ... https://git.kernel.org/linus/92eb6c3060eb

**11 memcpy() buffer overflow flaws ...** <div style="height:500px; width:900px; margin: auto;"> <canvas id="cves" data-chart="pie"> ,array index overflow (7),memcpy overflow (11),bad allocation size (2),iovec confusion (1),type confusion (1),string handling (1) CVEs, 7, 11, 2, 1, 1, 1, <!-- { "options": { "animation": false, "plugins": { "legend": { "position": "right", "labels": { "boxWidth": 50, "font": { "size": 32 } } } }, "backgroundColor": [ "rgba(82,215,38,.1)", "rgba(255,0,0,.9)", "rgba(255,236,0,.1)", "rgba(255,115,0,.1)", "rgba(0,126,214,.1)", "rgba(124,221,221,.1)" ] } } -->

### CVE-2020-24490 https://git.kernel.org/linus/a2ec905d1e16 https://google.github.io/security-research/pocs/linux/bleedingtooth/writeup#badvibes-heap-based-buffer-overflow-cve-2020-24490 [comment]: # "in process_adv_report() at net/bluetooth/hci_event.c:5537:" net/bluetooth/hci_event.c ```cpp struct discovery_state { ... u8 last_adv_data[HCI_MAX_AD_LENGTH]; ... }; ... memcpy(d->last_adv_data, data, len); ```

### CVE-2020-12654 https://git.kernel.org/linus/3a9b153c5591 [comment]: # "in mwifiex_ret_wmm_get_status() at drivers/net/wireless/marvell/mwifiex/wmm.c:992:" drivers/net/wireless/marvell/mwifiex/wmm.c ```cpp struct mwifiex_bssdescriptor { ... struct ieee_types_wmm_parameter wmm_ie; ... }; ... memcpy((u8 *) &priv->curr_bss_params.bss_descriptor.wmm_ie, wmm_param_ie, wmm_param_ie->vend_hdr.len + 2); ```

### CVE-2020-12653 https://git.kernel.org/linus/b70261a288ea [comment]: # "in mwifiex_cmd_append_vsie_tlv() at drivers/net/wireless/marvell/mwifiex/scan.c:2893:" drivers/net/wireless/marvell/mwifiex/scan.c ```cpp struct mwifiex_ie_types_vendor_param_set { ... u8 ie[MWIFIEX_MAX_VSIE_LEN]; ... }; ... memcpy(vs_param_set->ie, priv->vs_ie[id].ie, le16_to_cpu(vs_param_set->header.len)); ```

### CVE-2019-14895 https://git.kernel.org/linus/3d94a4a8373b [comment]: # "in mwifiex_process_country_ie() at drivers/net/wireless/marvell/mwifiex/sta_ioctl.c:252:" drivers/net/wireless/marvell/mwifiex/sta_ioctl.c ```cpp struct mwifiex_802_11d_domain_reg { ... struct ieee80211_country_ie_triplet triplet[MWIFIEX_MAX_TRIPLET_802_11D]; ... }; ... memcpy(domain_info->triplet, &country_ie[2] + IEEE80211_COUNTRY_STRING_LEN, country_ie_len); ```

### CVE-2019-14816 https://git.kernel.org/linus/7caac62ed598 [comment]: # "in mwifiex_update_vs_ie(): at drivers/net/wireless/marvell/mwifiex/ie.c:247" drivers/net/wireless/marvell/mwifiex/ie.c ```cpp struct mwifiex_ie { ... u8 ie_buffer[IEEE_MAX_IE_SIZE]; ... }; memcpy(ie->ie_buffer + le16_to_cpu(ie->ie_length), vs_ie, vs_ie->len + 2); ```

### CVE-2019-14815 https://git.kernel.org/linus/7caac62ed598 [comment]: # "in mwifiex_set_uap_rates() at drivers/net/wireless/marvell/mwifiex/uap_cmd.c:271:" drivers/net/wireless/marvell/mwifiex/uap_cmd.c ```cpp struct mwifiex_uap_bss_param { ... u8 rates[MWIFIEX_SUPPORTED_RATES]; ... }; ... memcpy(bss_cfg->rates, rate_ie + 1, rate_ie->len); ```

### CVE-2019-14814 https://git.kernel.org/linus/7caac62ed598 [comment]: # "in mwifiex_set_wmm_params() at drivers/net/wireless/marvell/mwifiex/uap_cmd.c:402:" drivers/net/wireless/marvell/mwifiex/uap_cmd.c ```cpp struct mwifiex_uap_bss_param { ... struct mwifiex_types_wmm_info wmm_info; ... }; ... memcpy(&bss_cfg->wmm_info, wmm_ie + sizeof(struct ieee_types_header), *(wmm_ie + 1)); ```

### CVE-2019-10126 https://git.kernel.org/linus/69ae4f6aac15 [comment]: # "in mwifiex_uap_parse_tail_ies() at drivers/net/wireless/marvell/mwifiex/ie.c:383:" drivers/net/wireless/marvell/mwifiex/ie.c ```cpp struct mwifiex_ie { ... u8 ie_buffer[IEEE_MAX_IE_SIZE]; ... }; ... memcpy(gen_ie->ie_buffer + ie_len, hdr, token_len); ```

### CVE-2019-9500 https://git.kernel.org/linus/1b5e2423164b [comment]: # "in brcmf_wowl_nd_results() at drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:3708:" drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c ```cpp struct cfg80211_ssid { u8 ssid[IEEE80211_MAX_SSID_LEN]; ... }; ... memcpy(cfg->wowl.nd->ssid.ssid, netinfo->SSID, netinfo->SSID_len); ```

### no CVE yet? https://git.kernel.org/linus/130f634da1af [comment]: # "in qtnf_event_handle_external_auth() at drivers/net/wireless/quantenna/qtnfmac/event.c:575:" drivers/net/wireless/quantenna/qtnfmac/event.c ```cpp struct cfg80211_ssid { u8 ssid[IEEE80211_MAX_SSID_LEN]; ... }; ... memcpy(auth.ssid.ssid, ev->ssid, len); ```

### no CVE yet? https://git.kernel.org/linus/d10a87a3535c [comment]: # "in wl1251_cmd_scan() at drivers/net/wireless/ti/wl1251/cmd.c:459:" drivers/net/wireless/ti/wl1251/cmd.c ```cpp struct cfg80211_ssid { u8 ssid[IEEE80211_MAX_SSID_LEN]; ... }; ... memcpy(cmd->params.ssid, ssid, ssid_len); ```

Wait a second, I thought **`CONFIG_FORTIFY_SOURCE=y`** solved this?!

Fortified `memcpy()` ```cpp[1-17|3-4|6-10|11-13|14|3-4] __FORTIFY_INLINE void *memcpy(void *dst, const void *src, size_t size) { size_t dst_size = __builtin_object_size(dst, 0); size_t src_size = __builtin_object_size(src, 0); if (__builtin_constant_p(size)) { /* Compile-time */ if (dst_size < size) __write_overflow(); if (src_size < size) __read_overflow2(); } if (dst_size < size || src_size < size) /* Run-time */ fortify_panic(__func__); return __underlying_memcpy(dst, src, size); } ```

`__builtin_object_size(OBJ, `**`MODE`**`)` MODE 0: bytes to end of entire structure MODE 1: bytes to end of structure *member* ```cpp[1-14|2,8,12|3,9,13|5,10,14] struct something { int count; /* 4 bytes */ char name[8]; /* 8 bytes */ int secret; /* 4 bytes */ char blob[]; /* flexible array */ } *instance; /* 16 bytes total */ __builtin_object_size(&instance->count, 0) == 16 __builtin_object_size(instance->name, 0) == 12 __builtin_object_size(instance->blob, 0) == -1 __builtin_object_size(&instance->count, 1) == 4 __builtin_object_size(instance->name, 1) == 8 __builtin_object_size(instance->blob, 1) == -1 ```

BleedingTooth targets members within the same composite structure... ```cpp[1-14|5|10] struct hci_dev { ... struct discovery_state { ... u8 last_adv_data[HCI_MAX_AD_LENGTH]; ... }; ... struct list_head { struct list_head *next; struct list_head *prev; } mgmt_pending; ... }; ``` ```cpp[1-5] static void store_pending_adv_report(struct hci_dev *hdev, ...) { struct discovery_state *d = &hdev->discovery; ... memcpy(d->last_adv_data, data, len); ```

Fortified `memcpy()` ```cpp[1-17|3-4] __FORTIFY_INLINE void *memcpy(void *dst, const void *src, size_t size) { size_t dst_size = __builtin_object_size(dst, 0); size_t src_size = __builtin_object_size(src, 0); if (__builtin_constant_p(size)) { /* Compile-time */ if (dst_size < size) __write_overflow(); if (src_size < size) __read_overflow2(); } if (dst_size < size || src_size < size) /* Run-time */ fortify_panic(__func__); return __underlying_memcpy(dst, src, size); } ```

MOAR Fortified `memcpy()` ```cpp[3-4] __FORTIFY_INLINE void *memcpy(void *dst, const void *src, size_t size) { size_t dst_size = __builtin_object_size(dst, 1); size_t src_size = __builtin_object_size(src, 1); if (__builtin_constant_p(size)) { /* Compile-time */ if (dst_size < size) __write_overflow(); if (src_size < size) __read_overflow2(); } if (dst_size < size || src_size < size) /* Run-time */ fortify_panic(__func__); return __underlying_memcpy(dst, src, size); } ```

Counting the memcpy() calls on a recent x86_64 allmodconfig build: - 35,294 memcpy() calls total: - 22,129 (62.7%) dest buffer size not known at compile time - 11,889 (33.7%) dest buffer size and copy size known at compile time - 1,276 (3.6%) dest buffer size known but copy size is dynamic In checking the 11 mentioned memcpy() flaws above, _all_ could be mitigated as part of adding dynamic copy size checks (1,276 instances), implying that we may have gained two orders of magnitude additional coverage over unknown flaws (11 were known and the remaining 1,265 more *might* be flaws), but where less than 4% of all memcpy() calls may be getting a false positive run-time warning.

But that would be too easy... Those 11,889 cases of "known buffer and copy size" actually needed to be fixed first, because of _sometimes_ intentional overflows. :( Luckily, they appear at compile time, so they can all be fixed.

Kernel had many intentional cross-member `memcpy()` overflows ... ```cpp[1-14|4,14|4-6,12,14] struct mwl8k_cmd_set_key { ... __u8 key_material[MAX_ENCR_KEY_LENGTH]; __u8 tkip_tx_mic_key[MIC_KEY_LENGTH]; __u8 tkip_rx_mic_key[MIC_KEY_LENGTH]; ... }; keymlen = MAX_ENCR_KEY_LENGTH + 2 * MIC_KEY_LENGTH; ... memcpy(cmd->key_material, key->key, keymlen); ```

... which could be a simple fix of adding a named struct ... ```cpp[3,7,14] struct mwl8k_cmd_set_key { ... struct { __u8 key_material[MAX_ENCR_KEY_LENGTH]; __u8 tkip_tx_mic_key[MIC_KEY_LENGTH]; __u8 tkip_rx_mic_key[MIC_KEY_LENGTH]; } tkip; ... }; keymlen = MAX_ENCR_KEY_LENGTH + 2 * MIC_KEY_LENGTH; ... memcpy(cmd->tkip, key->key, keymlen); ```

... but now everything must include the name of the named struct ... ```cpp[3,7,12-14] struct mwl8k_cmd_set_key { ... struct { __u8 key_material[MAX_ENCR_KEY_LENGTH]; __u8 tkip_tx_mic_key[MIC_KEY_LENGTH]; __u8 tkip_rx_mic_key[MIC_KEY_LENGTH]; } tkip; ... }; diff ... - do_something_with(cmd->key_material); + do_something_with(cmd->tkip.key_material); ```

... so invent `struct_group()` to provide both: ```cpp[3,7,12-14] struct mwl8k_cmd_set_key { ... struct_group(tkip, __u8 key_material[MAX_ENCR_KEY_LENGTH]; __u8 tkip_tx_mic_key[MIC_KEY_LENGTH]; __u8 tkip_rx_mic_key[MIC_KEY_LENGTH]; ); ... }; /* Accessible either way: */ do_something_with(cmd->key_material); do_something_with(cmd->tkip.key_material); ```

### `struct_group()` Creates a union of an anonymous and a named struct with identical members: ```cpp #define struct_group(NAME, MEMBERS...) \ union { \ struct { MEMBERS }; \ struct { MEMBERS } NAME; \ } ``` (There are additional helpers for adding attributes and tags.)

`memcpy()` run-time checking now possible! ```cpp[1-17|3-4|6-10|11-13] __FORTIFY_INLINE void *memcpy(void *dst, const void *src, size_t size) { size_t dst_size = __builtin_object_size(dst, 1); size_t src_size = __builtin_object_size(src, 1); if (__builtin_constant_p(size)) { /* Compile-time */ if (dst_size < size) __write_overflow(); if (src_size < size) __read_overflow2(); } if (dst_size < size || src_size < size) fortify_panic(__func__); /* Run-time */ return __underlying_memcpy(dst, src, size); } ```

So now we're done! Oh, wait, no, let's look at that list again: - 35,294 memcpy() calls total: - **22,129 (62.7%) dest buffer size not known at compile time** - 11,889 (33.7%) dest buffer size and copy size known at compile time - 1,276 (3.6%) dest buffer size known but copy size is dynamic

We need to deal with the flexible array case: ```cpp[3-4,7,9,12] __FORTIFY_INLINE void *memcpy(void *dst, const void *src, size_t size) { size_t dst_size = __builtin_object_size(dst, 1); size_t src_size = __builtin_object_size(src, 1); if (__builtin_constant_p(size)) { /* Compile-time */ if (dst_size < size) __write_overflow(); if (src_size < size) __read_overflow2(); } if (dst_size < size || src_size < size) fortify_panic(__func__); /* Run-time */ return __underlying_memcpy(dst, src, size); } ```

```cpp[1-9] struct bitmap_image { ... u16 pixels; /* how many items in flex array */ ... u32 pixel_data[]; /* flexible array */ } *image; __builtin_object_size(image->pixel_data, 1) == -1 sizeof(image->pixel_data) == invalid expression ``` So we just need to handle these. Except, wait for it, the kernel was filled with "fake" flexible arrays...

```cpp[5,8,9] struct bitmap_image { ... u16 pixels; /* how many items in flex array */ ... u32 pixel_data[0]; /* GNU extension: 0-element array */ } *image; __builtin_object_size(image->pixel_data, 1) == -1 sizeof(image->pixel_data) == 0 ``` Wait, what? Why aren't these both "0"? Shouldn't the new memcpy() refuse to write any bytes into this "fixed size" array?

```cpp[5,8,9] struct bitmap_image { ... u16 pixels; /* how many items in flex array */ ... u32 pixel_data[1]; /* Ancient style: 1-element array */ } *image; __builtin_object_size(image->pixel_data, 1) == -1 sizeof(image->pixel_data) == 4 ``` Oh, or this one? This was the even older way to specify a dynamically sized trailing array.

```cpp[5,8,9] struct bitmap_image { ... u16 pixels; /* how many items in flex array */ ... u32 pixel_data[64]; /* Actual fixed-size array... */ } *image; __builtin_object_size(image->pixel_data, 1) == -1 sizeof(image->pixel_data) == 256 ``` Oh no. _All trailing arrays_ are treated as "fake" flexible arrays. Only way out of this is to replace all fake flexible arrays with real flexible arrays, and then have the compilers enforce "no sloppy flexible arrays" ... which isn't even an option yet. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836

So, how to solve dynamically sized destination overflows? Compiler doesn't know the destination size. But the bounds are stored _somewhere_. For structures with flexible arrays, this is usually very nearby: ```cpp[1-6,3] struct bitmap_image { ... u16 pixels; ... u32 pixel_data[]; } *image; ``` Many call sites already check the bounds, but there are always going to be bugs with open-coded checks. And how many of these places remember to check the storage size of the bounds variable?

There have been proposals to add bounds annotation to the C language for flexible arrays, which would be absolutely amazing, but we don't have it today: ```cpp[3,5] struct bitmap_image { ... u16 pixels; ... u32 pixel_data[.pixels]; /* Compiler could reason about bounds! */ } *image; ``` This exists for other languages (e.g. Ada), so why not C? It provides a way for the developer to express their _intent_ to the compiler, which can then use it for sanity checking.

Instead, we must systematically enforce bounds checking with a new API, and _disallow `memcpy()` destinations that aren't a constant size_. ```cpp[1-12|7|9|10|11] struct bitmap_image { ... u16 pixels; ... u32 pixel_data[]; } *image; size_t count = width * height; image = kzalloc(struct_size(image, pixels, count), GFP_KERNEL); image->pixels = count; memcpy(image->pixel_data, screen, sizeof(u32) * count); ```

Toss the whole open-coded deserialization and use a new set of helpers designed to operate on "flexible array structures". ```cpp[1-12] struct bitmap_image { ... u16 pixels; ... u32 pixel_data[]; } *image; size_t count = width * height; // image = kzalloc(struct_size(image, pixels, count), GFP_KERNEL); // image->pixels = count; // memcpy(image->pixel_data, screen, sizeof(u32) * count); image = mem_to_flex_dup(image, pixel_data, pixels, screen, count, GFP_KERNEL); ```

And if the memory has already been allocated, different helpers are available: ```cpp[1-13] struct bitmap_image { ... u16 pixels; ... u32 pixel_data[]; } *image; size_t count = width * height; image = kzalloc(struct_size(image, pixels, count), GFP_KERNEL); image->pixels = count; // memcpy(image->pixel_data, screen, sizeof(u32) * count); mem_to_flex(image, pixel_data, pixels, screen, width * height); ```

`flex_clamp()`: Clamp size to data type storage ```cpp #define __flex_clamp(alloc, count_member, count) \ ({ \ size_t __fc_src_count = (count); \ size_t __fc_count_max = type_max(typeof((alloc)->count_member));\ if (__fc_src_count > __fc_count_max) { \ WARN_ONCE(IS_ENABLED(CONFIG_FORTIFY_SOURCE), \ "element count (%zu) cannot fit in " #alloc \ "->" #count_member " (max %zu): truncating\n",\ __fc_src_count, __fc_count_max); \ __fc_src_count = __fc_count_max; \ } \ __fc_src_count; \ }) ```

`mem_to_flex_dup()`: Allocate and deserialize into flex array struct ```cpp #define mem_to_flex_dup(alloc, dst, member, from_cpu, count_member, \ src, count, gfp) ({ \ typeof(*(alloc)) *__ptr; \ size_t __src_count = __flex_clamp(alloc, count_member, count); \ size_t __alloc_bytes = struct_size(__ptr, member, __src_count); \ __ptr = kmalloc(__alloc_bytes, gfp); \ if (__ptr) { \ /* here __copy_bytes will always be < __alloc_bytes */ \ size_t __copy_bytes = __alloc_bytes - \ offsetof(typeof(*__ptr), dst); \ memset(__ptr, 0, __alloc_bytes - __copy_bytes); \ __builtin_memcpy(&__ptr->dst, src, __copy_bytes); \ __ptr->count_member = from_cpu(__src_count); \ } \ __ptr; \ }) ```

`mem_to_flex()`: Deserialize into flex array struct ```cpp #define mem_to_flex(alloc, dst, member, to_cpu, from_cpu, count_member, src, count) \ ({ \ typeof(*(alloc)) *__ptr = (alloc); \ size_t __src_count = __flex_clamp(alloc, count_member, count); \ size_t __dst_count = to_cpu(__ptr->count_member); \ size_t __dst_bytes = struct_size(__ptr, member, __dst_count) - \ offsetof(typeof(*__ptr), dst); \ size_t __src_bytes = struct_size(__ptr, member, __src_count) - \ offsetof(typeof(*__ptr), dst); \ if (__dst_bytes < __src_bytes) { \ WARN_ONCE(IS_ENABLED(CONFIG_FORTIFY_SOURCE), \ "write (size %zu) into %s (size %zu) would " \ "overflow: truncating\n", __src_bytes, \ "field \"" #dst " -> " #member "\" at " \ __FILE__ ":" __stringify(__LINE__), __dst_bytes); \ __src_bytes = __dst_bytes; \ } \ __builtin_memcpy(__ptr->dst, src, __src_bytes); \ __ptr->count_member = from_cpu(__src_count); \ __ptr; \ }) ```

## Kernel release schedule - v5.16: _released January 9th_ - `struct_group()` (and related helpers) landed - almost all flexible array conversions done - v5.17: _March? (merge window now open)_ - all struct_group() conversions landed - almost all `-Warray-bounds` warnings fixed - v5.18: _May?_ - compile-time `memcpy()` enforcement landing - `mem_to_flex()` and related helpers landing - v5.19: _July?_ - run-time `memcpy()` enforcement landing

## Questions and Feedback? **Thank you for your attention!**   Kees ("Case") Cook @kees_cook keescook@chromium.org https://outflux.net/slides/2022/lca/