Messages in this thread | | | From | (Eric W. Biederman) | Date | Tue, 22 Dec 2020 11:33:58 -0600 | Subject | Re: Does uaccess_kernel() work for detecting kernel thread? |
| |
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:
> Commit db68ce10c4f0a27c ("new helper: uaccess_kernel()") replaced segment_eq(get_fs(), KERNEL_DS) > with uaccess_kernel(). But uaccess_kernel() became an unconditional "false" for some architectures > due to commit 5e6e9852d6f76e01 ("uaccess: add infrastructure for kernel builds with set_fs()") and > follow up changes in Linux 5.10. As a result, I guess that uaccess_kernel() can no longer be used > as a condition for checking whether current thread is a kernel thread or not. > > For example, if uaccess_kernel() is "false" due to CONFIG_SET_FS=n, > isn't sg_check_file_access() failing to detect kernel context? > > static int sg_check_file_access(struct file *filp, const char *caller) > { > if (filp->f_cred != current_real_cred()) { > pr_err_once("%s: process %d (%s) changed security contexts after opening file descriptor, this is not allowed.\n", > caller, task_tgid_vnr(current), current->comm); > return -EPERM; > } > if (uaccess_kernel()) { > pr_err_once("%s: process %d (%s) called from kernel context, this is not allowed.\n", > caller, task_tgid_vnr(current), current->comm); > return -EACCES; > } > return 0; > } > > For another example, if uaccess_kernel() is "false" due to CONFIG_SET_FS=n, > isn't TOMOYO unexpectedly checking permissions for socket operations? > > static bool tomoyo_kernel_service(void) > { > /* Nothing to do if I am a kernel service. */ > return uaccess_kernel(); > } > > static u8 tomoyo_sock_family(struct sock *sk) > { > u8 family; > > if (tomoyo_kernel_service()) > return 0; > family = sk->sk_family; > switch (family) { > case PF_INET: > case PF_INET6: > case PF_UNIX: > return family; > default: > return 0; > } > } > > Don't we need to replace such usage with something like (current->flags & PF_KTHREAD) ? > I don't know about io_uring, but according to > https://lkml.kernel.org/r/dacfb329-de66-d0cf-dcf9-f030ea1370de@schaufler-ca.com , > should (current->flags & (PF_KTHREAD | PF_IO_WORKER)) == PF_KTHREAD be used instead?
I think you are reading the situation properly.
I skimmed the tomoyo code and it appears that you are excluding kernel threads so as not to limit kernel threads such as nfsd. For PF_IO_WORKER kernel threads which are running code on behalf of a user we want to perform the ordinary permission checks. So you want the idiom you pasted above.
I do wonder though if perhaps we should create a is_user_cred helper to detect the difference between the creds of kernel threads and the thread of ordinary userspace. Which would handle io_uring that copy creds around and check them at a later time more cleanly.
Eric
| |