sys: implement Kernel CFI from clang

[aokblast]

Summary

This is a functional part for KCFI in FreeBSD

I have implemented a basic function which enable KCFI be triggered as expected.
Currently, we are able to boot in to os without any KCFI error emitted

What I think disable is reasonable so I disabled it in default:

1. link_elf_invoke_cbs, elf_lookup_ifunc: The kernel elf loader cannot know the actual return type of ifunc (they can only know ifunc will return a pointer), so the case of cbs.
2. ccfn in vsscanf: vsscanf may get strtoq or strtouq which return the different type
3. se->sy_call in syscallenter: kernel pack all parameter into a void * but the callee has the detailed type (For example: read_args, write_args).

What disable works but maybe able to fix:

1. callout parameter in fork_exit: don't know why broke so disabled now
2. post_ithread in ithread_execute_handlers: post_ithread callback callee have detailed type. But the caller use (void *) only. Don't know if breaks the current code for callee by letting callee use (void *) also is a good idea.

What needs discussion:

1. .m interface: D49113
2. eventhandler registration in kernel sometime doesn't follow the calling convention in eventhandler definition. Fixed in D49111
3. subsystems in kernel has their general error function. Takes sys/kern/vfs_default.c for example, we have vop_ebadf, vop_eopnotsupp, ...etc. : Fixed in current patch
4. vs->func in vnet_register_sysinit: take void * but caller has the detailed type fixed in D48490

My TODO List:
Check if the ud2 is triggerd by kcfi: fixed in current patch
Some file is not compiled with KCFI without setting CFLAG in kern.mk: fixed in current patch

[aokblast]

Fix format

[aokblast]

Format fix

[aokblast]

Cleanup the clang-format mis-instrument

[emaste]

stand/common/load_elf.c

728–729

sys/amd64/amd64/cfi.c

67

style(9) prefers not mixing comment styles - i.e. don't have some /* */ comments and some // comments

sys/kern/subr_cfi.c

2

Should have SPDX and copyright lines

[bz]

In D46193#1120552, @jhb wrote:

One general note is that it would probably be good to expand KCFI to "kernel CFI" at least the first time it is used in commit logs, etc.

In general it would be good to expand even CFI once in the commit message text (probably not subject) on first use.
We have too much acronym/abbreviation overload these days and people outside the domain have no clue... It's not the old term Canonical Format Indicator from 802.1q, and it's not the Common Flash Memory Interface also abbreviated CFI either... so should be Control-Flow Integrity? Just saying, had two other thoughts first.

sys/amd64/amd64/cfi.c

2

New file without copyright or license/SPDX?

[kib]

sys/amd64/amd64/cfi.c

3

If sys/systm.h is included, no need in cdefs.h or types.h

sys/kern/subr_cfi.c

2

cdefs.h is not needed

7

kassert.h is provided by systm.h

13

What is the reason for two #ifdef KERNEL blocks? And why this ifdef is needed at all for kernel source file?

33

Same etc ...

105

Don't you need to check that trapped instruction is indeed UD2?

[aokblast]

Fix

[kib]

stand/common/load_elf.c

741

De-indent the continuation line by 4 spaces.

sys/amd64/include/cfi.h

10 ↗

(On Diff #152759)

Why these prototypes cannot go into e.g. machine/trap.h?

sys/kern/link_elf.c

1337

If changing the comment from single-line into multi-line, follow style.

/*
 * Comment text.
 */

sys/kern/subr_module.c

409

I suggest to remove all that #ifdef KCFI braces, there and in parsing the module ELFs. The only thing that should be left under #ifdef is the actual code that does the calculations.

sys/sys/cfi.h

9 ↗

(On Diff #152759)

I think you can reduce namespace pollution by removing sys/proc.h (and probably sys/types.h as well), and just forward-declare

struct trapframe;

before cfi_handler() declaration.

12 ↗

(On Diff #152759)

Is there any cost or problem from always annotating functions with __NOCFI?

Also, consider moving this definition to sys/cdefs.h

17 ↗

(On Diff #152759)

Can this single prototype go into some existing header? IMO it does not make sense to add new header file for single prototype.

sys/sys/linker.h

115

Add these members unconditionally.

265

Remove this comment.

[lwhsu]

sys/amd64/amd64/cfi.c

6

"All rights reserved." is not needed anymore.

30

This is actually BSD-3-Clause license. If you want BSD-2-Clause, you can refer the first part of /COPYRIGHT

sys/kern/subr_cfi.c

6

"All rights reserved." is not needed anymore.

30

This is actually BSD-3-Clause license. If you want BSD-2-Clause, you can refer the first part of /COPYRIGHT

[aokblast]

sys/amd64/include/cfi.h

10 ↗

(On Diff #152759)

I discover amd64/traps.h only include x86 one and only put some #define inside.

Is it fine to move these two function declaration inside?

sys/sys/cfi.h

12 ↗

(On Diff #152759)

No, there is no cost. KCFI checks in forward egde ( The caller checks if callee is legal). So add this in caller side actually disable all check inside the caller function.

17 ↗

(On Diff #152759)

I see kcsan only has one prototype inside header too. I don't find any suitable place to put this signature. Or should we move these sanitizer into one header?

sys/sys/linker.h

115

If it fine to remove the ifdef in this structure? I think it may be somehow important?

[aokblast]

Minor fixes

[kib]

sys/amd64/amd64/cfi.c

71

Note that this is generally unsafe and might cause nested fault. You only know that the byte at tf_rip is valid and can be accessed because attempt to execute caused #UD and not #PF. But either byte before or after it might be not mapped.
You need to check that the accesses are safe before doing them, or place an exception handler around accesses using pcb_onfault.

90

This still does not check for UD2. UD2 is 0x0f 0x0b, you only check the second byte.

sys/amd64/include/cfi.h

10 ↗

(On Diff #152759)

Absolutely fine, just brace them with #ifdef _KERNEL.

sys/sys/cfi.h

17 ↗

(On Diff #152759)

Put it into e.g. sys/systm.h, which is the collection of the random things already. There is no harm from the unused prototype.

sys/sys/linker.h

115

It is highly desirable that kernel binary interface was invariant WRT config options. This is why I suggested to remove the #ifdef, to have the same struct linker_file size/layout without dependency on the option.

[aokblast]

sys/amd64/amd64/cfi.c

71

Could you please tell me the detail on how to achieve the safe access with

place an exception handler around accesses using pcb_onfault?

Since we cannot check if the address before ud2 and after ud2 is safe to access.

sys/amd64/include/cfi.h

10 ↗

(On Diff #152759)

Traps.h is used by .s files which cannot define structure inside. Can we put into the systm.h? Clang actually support KCFI on arm, riscv. But I only implement the x86 one.

[kib]

sys/amd64/amd64/cfi.c

71

Try D49566. I did not tested it.

sys/amd64/include/cfi.h

10 ↗

(On Diff #152759)

systm.h is fine.

[aokblast]

Rebase to main and use safe_read

[aokblast]

sys/amd64/amd64/cfi.c

71

It works as usual

[kib]

sys/amd64/amd64/cfi.c

72

You should not ignore errors from safe_read().

[aokblast]

sys/amd64/amd64/cfi.c

72

Oops.

switching to

for (idx = 0; idx < 14; idx++)                                                                                                             
          if (safe_read(tf->tf_rip - 12 + idx, buffer + idx))
              return false;

fail the KCFI check and make kernel go through the origianl path thus panic.

I can confirm that safe_read reads something as it passes all KCFI check.

[aokblast]

sys/amd64/amd64/cfi.c

72

I can confirm that safe_read reads something as it passes all KCFI check "in the version that does not check the error".

[kib]

sys/amd64/amd64/cfi.c

72

Still not done

80

Could you, instead of hardcoding the encoding in C, find the sequence in the compiled code? I mean, produce some label that points to the CFI sequence and compare the faulted instruction with it. Then if clang or clang-asm generate something different in binary, we do not even notice.

[aokblast]

sys/amd64/amd64/cfi.c

80

Does is_cfi_exception() match your expectation?

ALso, KCFI already provides a .cfi.traps section that contains all CFI fault PCs, so this code mainly serves as a fast path. I don’t think LLVM will change this sequence easily, but I understand your concern. I can remove the function entirely if needed.

[kib]

sys/amd64/amd64/cfi.c

80

Ok, no need to remove the function. I suggest to add the comment in line of your response to point out the section, and the purpose of the buffer matching.

sys/amd64/amd64/trap.c

535

This comment should go into the #ifdef KCFI block.

[aokblast]

Minor fixes

[aokblast]

sys/amd64/amd64/cfi.c

80

Ok. Thank you!

[kib]

sys/amd64/amd64/cfi.c

72

[aokblast]

Minor fixes

[kib]

stand/common/load_elf.c

740

sys/kern/link_elf.c

1344

[emaste]

sys/amd64/amd64/cfi.c

5

sys/kern/subr_cfi.c

5