set_memory_*() needs __must_check and atomicity

[kees]

Right now several architectures allow their set_memory_*() family of functions to fail, but callers may not be checking the return values. We need to fix the callers and add the __must_check attribute. They also may not provide any level of atomicity, in the sense that the memory protections may be left incomplete on failure.

Some additional details:
https://lore.kernel.org/netdev/[email protected]/

This issue likely has a few steps on effects architectures:

1. Have all callers of set_memory_*() helpers check the return value.
2. Add __much_check to all set_memory_*() helpers so that new uses do not ignore the return value.
3. Add atomicity to the calls so that the memory protections aren't left in a partial state.

(This issue depends on an architecture actually having implemented CONFIG_ARCH_HAS_STRICT_KERNEL_RWX.)

[mystictot]

Hi @kees
If is not assigned to anyone else may I assign it to me?

Please let me know.

Thanks a lot,
Shyam

[kees]

If is not assigned to anyone else may I assign it to me?

I think Tianlin Li is working on it, so please coordinate with them:
https://www.openwall.com/lists/kernel-hardening/2019/09/26/10

[tli16]

Hi @kees
I have a question about checking return values of set_memory_* in the calling functions.
What I am doing now is, if the return value is non-zero(which means set_memory_* function fails), then print the warning, and the caller returns immediately. If there is any memory allocated in the calling function, free the memory before return.
My concern is, if set_memory_* function fails, should the caller return? will it have some side effects? e.g. the caller of the caller assumes the execution is successful, no error checking.
Or should I just print warning without changing the return logic?

Thanks,
Tianlin

[kees]

The failure modes for each caller likely need to be examined and dealt with individually (e.g. what does it mean if something has failed?). Once those are done, then the __must_check attributes can be added.

[tli16]

Hi @kees,
When I am working on the patch of fixing the callers of set_memory_() functions, I touched 66 functions, and 45 of them just return void.
Ideally, the failure of set_memory_() should be passed up the call stack, but actually the callers are not concerned about the failure.

For example, set_memory_*() is invoked by frob_rodata, and frob_rodata is called by module_disable_ro, and module_disable_ro is called by klp_init_object_loaded. However, both frob_rodata and module_disable_ro are returning void.

One way is just printing warnings in frob_rodata() and return. But it may not be a good one.

The other way is, we should check for the error from module_disable_ro and return failure there like:
ret = module_disable_ro(patch->mod);
if (ret) {
mutex_unlock(&text_mutex);
return ret;
}
So we need to fix the return type of frob_rodata and module_disable_ro, and also all functions calling them.
The problem is there are 45 functions returning void that call set_memory_*(), and probably need walk multiple levels up to fix the return type. We may end up with patching hundreds of functions, and the patch will be super huge.

So could you please advise me the direction of this patch? I really appreciate it.

Best regards,
Tianlin

[kees]

Right, this will require many patches to plumb the failures. It will be a large series of patches when it's all done, but the change can be done incrementally a subsystem at a time. You can be sending those fixes to maintainers in parallel, and at some point in the future when all of them are plumbed, the __must_check attribute can be added.

Does that make sense? For example, you could do the module subsystem first and work out any issues there and then move on to other places.

[tli16]

Yeah that makes sense. I will start with module subsystem. Thank you!

[ajdlinux]

ping @ruscur, how does this affect your current patches to wire up set_memory_*() on powerpc?

[kees]

https://lore.kernel.org/lkml/[email protected]

[kees]

@tli16 Are you still working on this at all?

[chleroy]

Series proposed here for modules : https://patchwork.kernel.org/project/linux-modules/list/?state=*&series=812046

However, last patch had to be reverted because some ARM64 return -EINVAL when setting some memory to RO.

[chleroy]

After discussion with ARM64 architecture team a solution has been identified.

New patch available at: https://patchwork.kernel.org/project/linux-modules/patch/21037bf38438a285f5dff9501668f1675bc45989.1708070781.git.christophe.leroy@csgroup.eu/

[chleroy]

Set of patches for powerpc architecture:
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/b4907cf4339bd086abc40430d91311436cb0c18e.1708078401.git.christophe.leroy@csgroup.eu/
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/836f75710daef12dfea55f8fb6055d7fdaf716e3.1708078577.git.christophe.leroy@csgroup.eu/
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20ef75884aa6a636e8298736f3d1056b0793d3d9.1708078640.git.christophe.leroy@csgroup.eu/

[chleroy]

BPF :

• https://patchwork.kernel.org/project/netdevbpf/patch/63322c8e8454de9b240583de58cd730bc97bb789.1708165016.git.christophe.leroy@csgroup.eu/
• https://patchwork.kernel.org/project/netdevbpf/patch/135feeafe6fe8d412e90865622e9601403c42be5.1708253445.git.christophe.leroy@csgroup.eu/
• https://patchwork.kernel.org/project/netdevbpf/patch/ec35e06dbe8672a36415ebe2b9273277c2921977.1708253445.git.christophe.leroy@csgroup.eu/

PM :

• https://patchwork.kernel.org/project/linux-hardening/patch/be49c794567f3bd00ae16e91b54fe8cfe6cb4999.1708245599.git.christophe.leroy@csgroup.eu/