* Re: [PATCH] fixing userspace memory dereference in security.c
2023-12-16 4:11 ` [PATCH] fixing userspace memory dereference in security.c Serge E. Hallyn
@ 2023-12-18 23:06 ` Paul Moore
0 siblings, 0 replies; 2+ messages in thread
From: Paul Moore @ 2023-12-18 23:06 UTC (permalink / raw)
To: Serge E. Hallyn; +Cc: T. Williams, jmorris, linux-security-module, linux-kernel
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
^ permalink raw reply related [flat|nested] 2+ messages in thread