linux: a few fixes of callback typecasting (for the upcoming ClangCFI) #12260

solbjorn · 2021-06-20T20:25:01Z

Motivation and Context

ClangCFI (Control Flow Integrity) for ARM64 was accepted upstream during 5.13 merge window and will be widely available with 5.13 stable release.
ClangCFI for x86 is currently WIP, hopefully will hit 5.14 cycle.

CFI strongly prohibits callbacks typecasting as a part of possible code exploits/attacks.
On default configuration, every callback type mismatch leads to kernel panic, in "permissive" mode (which is designed for development and hunting mismatches/casting) this results in kernel warning splat. This can become a problem when ZFS will receive 5.13+ support.

Typical CFI failure splat in "permissive" mode:

[  535.456659] CFI failure (target: zio_execute.cfi_jt+0x0/0x8 [zfs]):
[  535.456820] Call Trace:
[  535.456822]  ? __cfi_check+0x1a7d/0x1e30 [zfs]
[  535.456882]  ? taskq_thread+0x6ce/0x750 [spl]
[  535.456893]  ? autoremove_wake_function.cfi_jt+0x8/0x8 [spl]
[  535.456900]  ? zil_lwb_write_done.cfi_jt+0x8/0x8 [zfs]
[  535.456960]  ? __thread_create.cfi_jt+0x10/0x10 [spl]
[  535.456967]  ? kthread+0x234/0x260
[  535.456969]  ? __thread_create.cfi_jt+0x10/0x10 [spl]
[  535.456976]  ? kthreadd.cfi_jt+0x8/0x8
[  535.456979]  ? ret_from_fork+0x22/0x30
[  535.456981] ---[ end trace 90952a0add50c989 ]---

Description

This PR might not cover 100% of callback type mismatches in ZFS kernel code, but at least fixes CFI failures on common tasks (pool/dataset creation/editing, booting Linux from ZFS root etc.).
The actual "fixes" are typecasting removals within as minimal changes as possible.

Covered code / CFI failures:

CFI failure (target: zio_execute.cfi_jt+0x0/0x8 [zfs]):
CFI failure (target: zpl_readpage.cfi_jt+0x0/0x10 [zfs]):
CFI failure (target: zil_itxg_clean.cfi_jt+0x0/0x10 [zfs]):
CFI failure (target: fnvlist_free.cfi_jt+0x0/0x10 [zfs]):
CFI failure (target: iput.cfi_jt+0x0/0x8 [zfs]):
CFI failure (target: xdr_u_longlong_t.cfi_jt+0x0/0x10 [znvpair]):
CFI failure (target: zio_interrupt.cfi_jt+0x0/0x8 [zfs]):
CFI failure (target: zvol_free.cfi_jt+0x0/0x8 [zfs]):

How Has This Been Tested?

booting Linux 5.12.12 on x86_64 with backported ClangCFI for ARM64 and x86 in "permissive" mode;
running common ZFS operations and tracking down CFI failure splats in kernel logs;
fixing the corresponding code;
booting in CFI "strict" mode and ensuring that the system works as expected;
running ZTS on x86_64 with CFI in "permissive" mode;
seeking for CFI failures in the kernel logs and fixing;
ensuring that they don't happen anymore.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

behlendorf

Interesting. Thanks for giving us a heads up on this so we can make the needed changes. There are likely a few more less common call paths in the code which we'll need to update. If you have easy access to a dedicated test system, and are so inclined, you could probably uncover the rest of these by locally running the ZFS Test Suite. The basic recipe can be found in the .github/workflows/zfs-tests-functional.yml file in the zfs repository. The test suite does perform some destructive operations, to to be extra safe I'd just make sure not to run it on a system which contains a pool with real data.

behlendorf · 2021-06-22T05:02:33Z

module/os/linux/zfs/zpl_file.c

+}
+
+static int
+zpl_readpage_filler(void *data, struct page *page)


nit: let's use pp for the page variable just for consistency with zpl_readpage().

solbjorn · 2021-06-22T09:33:10Z

Yeah, I think I could test it with ZFS Suite to catch more. I'll update this PR then, thanks!

solbjorn · 2021-06-24T23:38:03Z

@behlendorf, thanks for the idea, you was right. I've triggered some additional paths during the execution of ZFS Test Suite:

> grep 'CFI failure' log | cut -d';' -f2- | sort -u
CFI failure (target: fnvlist_free.cfi_jt+0x0/0x10 [zfs]):
CFI failure (target: iput.cfi_jt+0x0/0x8 [zfs]):
CFI failure (target: xdr_u_longlong_t.cfi_jt+0x0/0x10 [znvpair]):
CFI failure (target: zio_interrupt.cfi_jt+0x0/0x8 [zfs]):
CFI failure (target: zvol_free.cfi_jt+0x0/0x8 [zfs]):

Rough total amount of failures during ZTS cycle:

> grep 'CFI failure' log | cut -d';' -f2- | wc -l  
1602

I'll update this PR till the end of this week by covering the abovementioned code.

Instead of declaring zio_{,re}execute() as void (*)(zio_t *zio) and then typecasting it to void (*)(void *arg) when adding a taskq, declare it with void *arg initially. This implies no functional changes, but eliminates callback typecasting / type violation, which may be fatal in case of ClangCFI. Signed-off-by: Alexander Lobakin <[email protected]>

Just declare zil_itxg_clean() with void *arg instead of itxs_t * to be able to pass it directly to taskq_dispatch() without typecasting. No functional changes, but critical for passing ClangCFI checks. Signed-off-by: Alexander Lobakin <[email protected]>

To avoid callback typecast violations (and e.g. triggering ClangCFI), declare separate callbacks for read_cache_pages() and zpl_address_space_operations: - convert zpl_readpage() to a static inline zpl_readpage_common(), which now takes only one argument instead of two (the first was never used in the code); - declare zpl_readpage() for zpl_address_space_operations and zpl_readpage_filler() to pass it as read_cache_pages() agrument. They differ only by the type of the first argument (struct file * vs void *), and both just inline zpl_readpage_common(). - Pass NULL as the last argument (void *data) of read_cache_pages() call as zpl_readpage_filler() (former zpl_readpage()) doesn't use it. Signed-off-by: Alexander Lobakin <[email protected]>

Just declare zio_interrupt() as void (*)(void *) to match task_func_t type and pass callback typecheck. This doesn't change anything for the direct callers. Signed-off-by: Alexander Lobakin <[email protected]>

solbjorn · 2021-06-26T20:28:49Z

Updated. Now covers several more cases:

CFI failure (target: fnvlist_free.cfi_jt+0x0/0x10 [zfs]):
CFI failure (target: iput.cfi_jt+0x0/0x8 [zfs]):
CFI failure (target: xdr_u_longlong_t.cfi_jt+0x0/0x10 [znvpair]):
CFI failure (target: zio_interrupt.cfi_jt+0x0/0x8 [zfs]):
CFI failure (target: zvol_free.cfi_jt+0x0/0x8 [zfs]):

and passes the entire ZTS with not a single CFI failure.
checkstyle warning and a review comment also were addressed.

behlendorf · 2021-06-29T19:29:48Z

@solbjorn this is looking good! It looks like there's perhaps just one last cstyle warning to resolve.

At the moment, nvs_xdr_nvp_op() passes several static inlines as callbacks for xdr_array(). This implicitly creates static functions for each of them to be able to retrieve their addresses. However, all those functions have different prototypes incompatible with xdrproc_t which involves callback typecasting and CFI failures. Just declare the callbacks explicitly via a shorthand helper. This doesn't increase the codesize, but allows to pass function addresses directly to xdr_array(). Signed-off-by: Alexander Lobakin <[email protected]>

Wrap the external iput() into a static callback and pass it to taskq_dispatch() instead of passing it directly with typecasting. Signed-off-by: Alexander Lobakin <[email protected]>

Wrap it into a static function and pass it direcly as a zcp_cleanup_t callback. Signed-off-by: Alexander Lobakin <[email protected]>

Wrap it into a static function and pass it as a task callback instead. ops is dereferenced prior to this, so check for ops != NULL is not needed. Signed-off-by: Alexander Lobakin <[email protected]>

solbjorn · 2021-06-29T19:58:20Z

Updated. Those are xdr_u_short() and xdr_u_int() function suffixes, not the real type usages, so I suppressed these two false-positives with CSTYLED.

solbjorn · 2021-07-19T21:09:39Z

Ping? Was approved 19 days ago...

* zio: avoid callback typecasting * zil: avoid zil_itxg_clean() callback typecasting * zpl: decouple zpl_readpage() into two separate callbacks * nvpair: explicitly declare callbacks for xdr_array() * linux/zfs_nvops: don't use external iput() as a callback * zcp_synctask: don't use fnvlist_free() as a callback * zvol: don't use ops->zv_free() as a callback for taskq_dispatch() Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Signed-off-by: Alexander Lobakin <[email protected]> Closes openzfs#12260

* zio: avoid callback typecasting * zil: avoid zil_itxg_clean() callback typecasting * zpl: decouple zpl_readpage() into two separate callbacks * nvpair: explicitly declare callbacks for xdr_array() * linux/zfs_nvops: don't use external iput() as a callback * zcp_synctask: don't use fnvlist_free() as a callback * zvol: don't use ops->zv_free() as a callback for taskq_dispatch() Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Signed-off-by: Alexander Lobakin <[email protected]> Closes #12260

* zio: avoid callback typecasting * zil: avoid zil_itxg_clean() callback typecasting * zpl: decouple zpl_readpage() into two separate callbacks * nvpair: explicitly declare callbacks for xdr_array() * linux/zfs_nvops: don't use external iput() as a callback * zcp_synctask: don't use fnvlist_free() as a callback * zvol: don't use ops->zv_free() as a callback for taskq_dispatch() Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Signed-off-by: Alexander Lobakin <[email protected]> Closes openzfs#12260

behlendorf reviewed Jun 22, 2021

View reviewed changes

behlendorf added the Status: Code Review Needed Ready for review and testing label Jun 22, 2021

solbjorn added 4 commits June 26, 2021 22:02

zio: avoid zio_interrupt() callback typecasting

8150edd

Just declare zio_interrupt() as void (*)(void *) to match task_func_t type and pass callback typecheck. This doesn't change anything for the direct callers. Signed-off-by: Alexander Lobakin <[email protected]>

ahrens assigned mmaybee Jun 28, 2021

solbjorn added 4 commits June 29, 2021 21:56

linux/zfs_nvops: don't use external iput() as a callback

ce1c398

Wrap the external iput() into a static callback and pass it to taskq_dispatch() instead of passing it directly with typecasting. Signed-off-by: Alexander Lobakin <[email protected]>

zcp_synctask: don't use fnvlist_free() as a callback

025078e

Wrap it into a static function and pass it direcly as a zcp_cleanup_t callback. Signed-off-by: Alexander Lobakin <[email protected]>

zvol: don't use ops->zv_free() as a callback for taskq_dispatch()

372a847

Wrap it into a static function and pass it as a task callback instead. ops is dereferenced prior to this, so check for ops != NULL is not needed. Signed-off-by: Alexander Lobakin <[email protected]>

behlendorf approved these changes Jul 1, 2021

View reviewed changes

mmaybee approved these changes Jul 20, 2021

View reviewed changes

mmaybee added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jul 20, 2021

mmaybee merged commit 23c13c7 into openzfs:master Jul 20, 2021

solbjorn deleted the fixes-for-cfi branch July 22, 2021 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

linux: a few fixes of callback typecasting (for the upcoming ClangCFI) #12260

linux: a few fixes of callback typecasting (for the upcoming ClangCFI) #12260

Uh oh!

solbjorn commented Jun 20, 2021 •

edited

Loading

Uh oh!

behlendorf left a comment

Uh oh!

behlendorf Jun 22, 2021

Uh oh!

solbjorn commented Jun 22, 2021

Uh oh!

solbjorn commented Jun 24, 2021

Uh oh!

solbjorn commented Jun 26, 2021

Uh oh!

behlendorf commented Jun 29, 2021

Uh oh!

solbjorn commented Jun 29, 2021

Uh oh!

solbjorn commented Jul 19, 2021

Uh oh!

Uh oh!

linux: a few fixes of callback typecasting (for the upcoming ClangCFI) #12260

linux: a few fixes of callback typecasting (for the upcoming ClangCFI) #12260

Uh oh!

Conversation

solbjorn commented Jun 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

behlendorf left a comment

Choose a reason for hiding this comment

Uh oh!

behlendorf Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

solbjorn commented Jun 22, 2021

Uh oh!

solbjorn commented Jun 24, 2021

Uh oh!

solbjorn commented Jun 26, 2021

Uh oh!

behlendorf commented Jun 29, 2021

Uh oh!

solbjorn commented Jun 29, 2021

Uh oh!

solbjorn commented Jul 19, 2021

Uh oh!

Uh oh!

solbjorn commented Jun 20, 2021 •

edited

Loading