Messages in this thread Patch in this message | | | From | Paul Moore <> | Date | Mon, 18 Dec 2023 18:06:27 -0500 | Subject | Re: [PATCH] fixing userspace memory dereference in security.c |
| |
On Fri, Dec 15, 2023 at 11:11 PM Serge E. Hallyn <serge@hallyn.com> wrote: > On Wed, Oct 06, 2021 at 07:03:56PM -0400, T. Williams wrote: > > security/security.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/security/security.c b/security/security.c > > index 9ffa9e9c5c55..7c41b5d732ab 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -1737,6 +1737,8 @@ int security_kernel_read_file(struct file *file, enum > > kernel_read_file_id id, > > int ret; > > > > ret = call_int_hook(kernel_read_file, 0, file, id, contents); > > + if (ret > 0) > > + return -EINVAL; > > if (ret) > > return ret; > > return ima_read_file(file, id, contents); > > -- > > 2.25.1 > > > > This commit is to fix a userspace address dereference found by > > syzkaller. > > The crash is triggered by passing a file descriptor to an incorrectly > > formed kernel module to finit_module. > > > > Kernel/module.c:4175 : Within the finit_module, info.len is set to the > > return value from kernel_read_file_from_fd. This value is then > > dereferenced by memcmp within module_sig_check from inside load_module. > > The value is 0xb000000 so the kernel dereferences user memory and kernel > > panics. > > Hi, > > thanks for sending this. For some reason, I can't seem to find this > message-id in lore.kernel.org to see if there were ever any replies.
I'm not sure where the original email/patch was sent, but I don't seem to have a copy in my inbox either. Odd.
> There is indeed a problem, although I think a more concise explanation > is: > > 1. security_kernel_read_file() returns any non-zero return value to mean > permission denied > 2. kernel_read_file() returns > 0 meaning number of bytes read > 3. hen kernel_read_file() gets any non-zero rv from security_kernel_read_file(), > it returns that value unchanged. > > Since kernel_read_file() is the only caller of security_kernel_read_file(), > I think your patch is good, except you should also change the comment above > it to read > > * Return: Returns 0 if permission is granted, < 0 on error. > > Reviewed-by: Serge Hallyn <serge@hallyn.com> > > I think the reason it's not been a practical problem is because while > security_kernel_read_file() will honor a >0 error from an LSM, no > LSM implementation of that hook does that. (Only loadpin and selinux > implement it)
The SELinux implementation should only ever return 0 or a negative value, and based on a quick look at Loadpin I would say the same applies there as well. This patch doesn't address the IMA hook, but according to the comments in the IMA code, it too should only return 0 or a negative value. While it is theoretically possible that security_kernel_read_file() could return a positive value, I'm missing where/how that might happen. Help?
That said, I agree with Serge that this is worth fixing, and in addition to the comment suggestion from Serge, I would ask that you fix the IMA hook too. I would expect the patch to look something like this:
diff --git a/security/security.c b/security/security.c index dcb3e7014f9b..dd8bdda166f3 100644 --- a/security/security.c +++ b/security/security.c @@ -3043,7 +3043,7 @@ int security_kernel_module_request(char *kmod_name) * * Read a file specified by userspace. * - * Return: Returns 0 if permission is granted. + * Return: Returns 0 if permission is granted, negative values on failure. */ int security_kernel_read_file(struct file *file, enum kernel_read_file_id id, bool contents) @@ -3052,8 +3052,15 @@ int security_kernel_read_file(struct file *file, enum ker nel_read_file_id id,
ret = call_int_hook(kernel_read_file, 0, file, id, contents); if (ret) + goto out; + ret = ima_read_file(file, id, contents); + +out: + if (ret > 0) + return -EINVAL; + if (ret < 0) return ret; - return ima_read_file(file, id, contents); + return 0; } EXPORT_SYMBOL_GPL(security_kernel_read_file);
-- paul-moore.com
| |