Date: Mon, 18 Dec 2023 18:06:27 -0500
From: Paul Moore <paul@...l-moore.com>
To: "Serge E. Hallyn" <serge@...lyn.com>
Cc: "T. Williams" <tdwilliamsiv@...il.com>, jmorris@...ei.org, 
	linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fixing userspace memory dereference in security.c

On Fri, Dec 15, 2023 at 11:11 PM Serge E. Hallyn <serge@...lyn.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@...lyn.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

Powered by blists - more mailing lists