CFI violation in rm_kernel_rmapi_op

[sempervictus]

NVIDIA Open GPU Kernel Modules Version

525.60.13

Does this happen with the proprietary driver (of the same version) as well?

I cannot test this

Operating System and Version

Arch Linux Current

Kernel Release

6.0.16

Hardware: GPU

A5000

Describe the bug

When building the kernel (and therefore all modules) using RAP CFI, the open GPU modules code actually compiles but the nvidia module cannot be loaded into the kernel because it has a CFI violation in rm_kernel_rmapi_op: RAP hash rm_kernel_rmapi_op/3262ced1 for rm_kernel_rmapi_op+0x0/0x14d [nvidia] does not match existing hash 1f0fc3eb00017f.
The function is defined as void NV_API_CALL rm_kernel_rmapi_op(nvidia_stack_t *sp, void *ops_cmd) with consistent calling conventions (to my naked eye) throughout the code, so this doesnt look to be a case of overridden/colliding names. The NV_API_CALL definition also looks like it shouldn't change during compilation.
Do any of the developers know whether something its doing, or its compiled, or the way that its called could break call or return hash checks?

On a related note - the stackprotector appears to be used when its detected as available, not when its detected as having been used to build the rest of the kernel (RAP obviates the need for SSP when built with return checks, so while its available to the compiler, its not enabled during full-RAP builds).

To Reproduce

Build module against a RAP-instrumented kernel, then try to insmod

Bug Incidence

Always

nvidia-bug-report.log.gz

N/A

More Info

No response

[timocapa]

Hit the same

CFI failure at nvkms_call_rm+0x5b/0xa0 [nvidia_modeset] (target: rm_kernel_rmapi_op+0x0/0x190 [nvidia]; expected type: 0xba54dd86)

Another one is

CFI failure at nvkms_kthread_q_callback+0x101/0x140 [nvidia_modeset] (target: _nv000067kms+0x0/0x10 [nvidia_modeset]; expected type: 0xe1419545)

(Clang CFI + nvidia-dkms)

Clang 16 (built from master a few days ago), so this is likely kCFI

[sempervictus]

Thanks for chiming in @timocapa. Given that we're using two completely different CFI mechanisms (clang vs a GCC plugin on my end), i think this is a strong indication of the driver doing something "unsavory" at runtime despite meeting both implementations' build-time requirements.
I'm guessing that i just never get to the stack frame with the nvkms_kthread_q_callback call/access since the early failure prevents anything else from executing after the first failure.

[timocapa]

I'm guessing that i just never get to the stack frame with the nvkms_kthread_q_callback call/access since the early failure prevents anything else from executing after the first failure

Perhaps - I put kCFI into permissive mode, so failures get logged but execution continues

[sempervictus]

Ping @aritger - your name is on all the release tags, so pinging direct to get upstream attention on this.

My situation w/ RAP might not be common enough to merit upstream remediation, but with kernel CFI as a technique becoming more mainstream (in general Linux and Android), i think that this issue occurring on various CFI implementations does merit attention from Nvidia folks... The day some distro starts shipping KCFI will be the day when these drivers are prevented from loading at all on an common OS. End-user markets are one thing, but a fair number of our mutual customers are hospitals, universities, pharma, etc - institutional buyers who have to operate under their GRC mandates (to use state of the art exploit mitigation techniques and not disable security functions which is what kCFI permissive mode does).

[aritger]

Thanks for the report.

I apologize for my ignorance, but what is an easy way for me to configure a RAP-instrumented kernel? And/or how to configure the kernel and open-gpu-kernel-modules to build with Clang CFI?

For rm_kernel_rmapi_op:

• NV_API_CALL should be #defined to nothing in open-gpu-kernel-modules builds.

• Both the implementation and callers should agree on the signature:

    void       NV_API_CALL rm_kernel_rmapi_op(nvidia_stack_t *sp, void *ops_cmd);

• rm_kernel_rmapi_op is interesting in that nvidia.ko uses it in a function pointer assignment in a const structure in kernel-open/nvidia/nv-modeset-interface.c:nvidia_get_rm_ops():

        const nvidia_modeset_rm_ops_t local_rm_ops = {
            [...]
            .op             = rm_kernel_rmapi_op, /* provided by nv-kernel.o */
            [...]
        };

• nvidia_get_rm_ops() uses the above 'local_rm_ops' const local variable to assign the passed-in nvidia_modeset_rm_ops_t*:

        *rm_ops = local_rm_ops;

• nvidia_get_rm_ops() is an EXPORT_SYMBOL()ed function, which nvidia-modeset.ko calls.

• nvidia-modeset.ko then calls through the function pointer in kernel-open/nvidia-modeset/nvidia-modeset-linux.c:nvkms_call_rm()

            __rm_ops.op(stack, ops);

(&__rm_ops is the nvidia_modeset_rm_ops_t* that nvidia-modeset.ko passes into nvidia_get_rm_ops())

For nvkms_kthread_q_callback:

• This doesn't appear to use __rm_ops, like the rm_kernel_rmapi_op case.

• It /does/ call through the function pointer nvkms_timer_t::proc, with signature nvkms_timer_proc_t, which is:

    typedef void nvkms_timer_proc_t(void *dataPtr, NvU32 dataU32);

As far as I can tell, the caller and functions that get assigned to ::proc agree on the signature.

Is any of this a violation of CFI?

[sempervictus]

Thanks for picking this up @aritger.
Far as RAP goes, the last public release is from some years ago, so for "easy setup" you'd need an old kernel.
However, since the problem is reproducible using currently publicly available tooling as indicated by @timocapa, i would suggest going the kCFI route via LLVM/clang to create relevant instrumentation in the runtime. Moreover, RAP isn't permissive, so kCFI seems a better way to find occurrences of the concern.
I can build any kCFI-derived remedy with my RAP plugin and currently installed kernel to verify the remedy working there as well (should ensure that the fix is not reliant on some version-specific implementation of clang's CFI).

My gut sense is that the "call through" approach is what's tripping up CFI, but need to consult with people better informed on the matter... Hopefully they're able to weigh-in directly.

[timocapa]

And/or how to configure the kernel and open-gpu-kernel-modules to build with Clang CFI?

The Linux kernel should take setting LLVM=1 make as argument to fully switch to Clang's utilities - CFI is disabled by default so you'll have to enable its config. Clang versions newer than 14.0.x will likely hit errors due to new warnings turning into errors changes. (https://github.com/gentoo/gentoo/blob/master/x11-drivers/nvidia-drivers/files/nvidia-drivers-525.23-clang15.patch)

I've used Clang 16 which I can upload later.

If setting LLVM=1 won't work for the NVIDIA driver, you'll likely just override CC and CXX I guess? Not sure, I can only use the proprietary driver due to having a Pascal GPU - DKMS has been updated a while ago to try to match the compiler that was used for the kernel

Other than that I don't know enough about CFI to answer the other questions, sorry. :(

[sempervictus]

Regarding the CFI piece - apparently the types of pointers being passed as arguments to functions must be understood by the compiler when creating hashes for calls and returns. Structure definitions present in kernel-open but missing from src/nvidia or the other way around will produce different representations in CFI's comprehension of "what its seeing in the code." That's the current running theory anyway. There seems to be a linker hack-around for this concern already in-place @ src/nvidia/exports_link_command.txt which i imagine also prevents LTO from "being all that it can be" since much like CFI, it needs to be type-aware to perform its optimizations.

Is the "partitioning" of the code set up to produce some sort of functional benefit in compilation, or a byproduct of the open-sourcing process slated for remedy at some point?

[sempervictus]

@artiger: wiser folks than me have noticed that the module is not being built using Kbuild (which explains why i had the SSP problem mentioned in the original post). Meaning that the hash showing up as a mismatch in my error output just some random segment of the binary...

[pageexec]

just my 2 cents: the core issue is that the whole build system is structured the 'wrong way' and results in objects with mixed provenance linked together. namely, some objects are produced by the kernel's Kbuild system and some are not. how this can possibly work is a mystery, just think about RANDSTRUCT being applied to one set of objects but not the others, etc.

[pageexec]

in more details: Makefile defines the modules target as:
58 modules: $(nv_kernel_o_binary) $(nv_modeset_kernel_o_binary)
59 »·······$(MAKE) -C kernel-open modules

here kernel-open/Makefile will end up using the kernel's Kbuild system to build the objects, which is how it should be done for everything else too. however nv_kernel_o_binary is built this way:
33 $(nv_kernel_o):
34 »·······$(MAKE) -C src/nvidia

which in turn ends up in src/nvidia/Makefile which has nothing to do with Kbuild and just builds the objects with some made up CFLAGS that have no relationship with the rest of the kernel's KCFLAGS. this is just broken, it'll not produce anything that will have CFI/etc instrumentations. so before spending too much time on these supposed CFI violations (they are not, as far as I checked), fix the build system first then start looking for problems.

[pageexec]

small bonus, at compile time, RAP caught a prototype mismatch on nv_encode_caching (between kernel-open/common/inc/nv-proto.h and kernel-open/nvidia/nv-mmap.c).

[sempervictus]

@pageexec - thanks for jumping in at whatever ungodly time it is over there. That mismatch is just a matter of

diff --git c/kernel-open/common/inc/nv-proto.h w/kernel-open/common/inc/nv-proto.h
index 815107d25..b016e42e6 100644
--- c/kernel-open/common/inc/nv-proto.h
+++ w/kernel-open/common/inc/nv-proto.h
@@ -43,7 +43,7 @@ void        nv_procfs_remove_gpu        (nv_linux_state_t *);
 
 int         nvidia_mmap                 (struct file *, struct vm_area_struct *);
 int         nvidia_mmap_helper          (nv_state_t *, nv_linux_file_private_t *, nvidia_stack_t *, struct vm_area_struct *, void *);
-int         nv_encode_caching           (pgprot_t *, NvU32, NvU32);
+int         nv_encode_caching           (pgprot_t *, NvU32, nv_memory_type_t);

i've lost track of what's already in your patch and what i've added since, but happy to send over what i have currently.

@aritger: ^^ is one of said wiser folks to whom i was referring. 😁
I'm pretty sure that the build conditions not being consistent are also what resulted in my having to

commit c0f1b8eabb1a1cfb0b401848dd65b4e30114a7e3 (HEAD -> 525.60.13-grsec)
Author: RageLtMan <rageltman [at] sempervictus>
Date:   Wed Dec 28 16:07:00 2022 -0500

    Force no-SSP to avoid RAP conflict

diff --git a/utils.mk b/utils.mk
index 17421ba9f..0ff35f725 100644
--- a/utils.mk
+++ b/utils.mk
@@ -39,7 +39,7 @@ AR                    ?= ar
 # only set these warnings if CFLAGS is unset
 CFLAGS                ?= -Wall
 # always set these -f CFLAGS
-CFLAGS                += -fno-strict-aliasing -fno-omit-frame-pointer -Wformat=2
+CFLAGS                += -fno-strict-aliasing -fno-omit-frame-pointer -Wformat=2 -fno-stack-protector
 CC_ONLY_CFLAGS        ?=
 CXX_ONLY_CFLAGS       ?=
 LDFLAGS               ?=

in order to build when i'm using RAP because RAP (with return protection) disables the Kconfig for stack protection but the build system still tries to use it (when the rest of the kernel doesn't) resulting in a compile failure without the hack above.

[aritger]

Thanks for the feedback. That is a fair critique.

For whatever it is worth, there are a few motivations for the split:

(1) Historically, the non-kbuild part (the part that produces nv-kernel.o) was built internally to NVIDIA and is what was distributed as binary-only. Code not built for a specific target kernel cannot use kbuild.

(2) With the advent of open-gpu-kernel-modules, we chose to retain that split so that users installing the driver wouldn't be required to build all of the kernel module when installing the driver. I.e., installing the driver from the NVIDIA .run file contains a pre-built open-gpu-kernel-modules nv-kernel.o. We can only do that because nv-kernel.o is not kernel-specific. Currently, open-gpu-kernel-modules takes about 10 minutes to build if single threaded. Much of that can be covered with a parallel build, but we didn't want to add that install time for every user installing from .run file if we didn't need to.

The big disadvantage of the split is of course that you need to match these sorts of compiler flags across the split if doing instrumentation like RAP.

Maybe the benefits of (2) are outweighed by the downsides and we should revisit that decision.

That is at least the context. So, I don't know if we can immediately move to an all kbuild-native build.

The nv_encode_caching() bug is a good catch. Thanks for that. Does nv-mmap.c not include nv-proto.h? If not, that is a bug, too. Even with the current split, I would expect the compiler to complain if the prototype and implementation mismatch.

For the near-term, would it be acceptable to pass these additional CFLAGS on the make commandline? Maybe the makefiles need more variable plumbing to facilitate that. But, I think it will be easiest to get traction with something like that, than require kbuild-ifying the entirety of the open-gpu-kernel-modules build. The code changes for that wouldn't be difficult, but the hard part would be the packaging/installation implications of that choice.