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.
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 <[email protected]>
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)
-serge
On Fri, Dec 15, 2023 at 11:11 PM Serge E. Hallyn <[email protected]> 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 <[email protected]>
>
> 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