buffer overflow in vpe_write() function of arch/mips/kernel/vpe.c

Commit Message

Ning Qiang July 14, 2022, 1:12 p.m. UTC

In the vpe_write function of arch/mips/kernel/vpe.c, parameter "size_t count" is pass by userland, if "count" is very large, it will bypass the check of "if ((count + v->len) > v->plen)".(such as count=0xffffffffffffffff). Then it will lead to buffer overflow in "copy_from_user(v->pbuffer + v->len, buffer, count)".

Comments

Greg KH July 14, 2022, 1:31 p.m. UTC | #1

On Thu, Jul 14, 2022 at 09:12:38PM +0800, sohu0106 wrote:
> 
> In the vpe_write function of arch/mips/kernel/vpe.c, parameter "size_t count" is pass by userland, if "count" is very large, it will bypass the check of "if ((count + v->len) > v->plen)".(such as count=0xffffffffffffffff). Then it will lead to buffer overflow in "copy_from_user(v->pbuffer + v->len, buffer, count)".
> 
> 
> diff --git a/arch/mips/kernel/vpe.c_org b/arch/mips/kernel/vpe.c
> index d0d832a..bd1f826 100644
> --- a/arch/mips/kernel/vpe.c_org
> +++ b/arch/mips/kernel/vpe.c
> @@ -871,7 +871,7 @@ static ssize_t vpe_write(struct file *file, co  nst char __user *buffer,
>         if (v == NULL)
>                 return -ENODEV;
> 
> -       if ((count + v->len) > v->plen) {
> +       if ((count + v->len) > v->plen || count + v->len > v->len)   {
>                 pr_warn("VPE loader: elf size too big. Perhaps str  ip unneeded symbols\n");
>                 return -ENOMEM;
>         }
> 

Note, there is no need to cc: security@k.o when you also cc: public
mailing lists, as the developers there should handle the issue.

Also, your patch is corrupted with whitespace turned into spaces, making
it impossible to apply, and you forgot a signed-off-by line.  Try
running scripts/checkpatch.pl on your patches before sending them out,
that will catch things like this.

thanks,

greg k-h

Dan Carpenter July 14, 2022, 1:42 p.m. UTC | #2

On Thu, Jul 14, 2022 at 09:12:38PM +0800, sohu0106 wrote:
> 
> In the vpe_write function of arch/mips/kernel/vpe.c, parameter "size_t count" is pass by userland, if "count" is very large, it will bypass the check of "if ((count + v->len) > v->plen)".(such as count=0xffffffffffffffff). Then it will lead to buffer overflow in "copy_from_user(v->pbuffer + v->len, buffer, count)".
> 
> 
> diff --git a/arch/mips/kernel/vpe.c_org b/arch/mips/kernel/vpe.c
> index d0d832a..bd1f826 100644
> --- a/arch/mips/kernel/vpe.c_org
> +++ b/arch/mips/kernel/vpe.c
> @@ -871,7 +871,7 @@ static ssize_t vpe_write(struct file *file, co  nst char __user *buffer,
>         if (v == NULL)
>                 return -ENODEV;
> 
> -       if ((count + v->len) > v->plen) {
> +       if ((count + v->len) > v->plen || count + v->len > v->len)   {

This integer overflow check is wrong.  It should be < instead of >.
Also do the integer overflow check first before the other check.  I
would normally write it like so:

	if (count > ULONG_MAX - v->len ||
	    count + v->len > v->plen) {
		pr_warn("VPE loader: elf size too big. Perhaps str  ip unneeded symbols\n");
		return -ENOMEM;
	}

There are some other mechanical issues with the patch.

1) It needs a subsystem prefix:

Subject: [PATCH] MIPS: vpe: fix integer overflow in vpe_write()

2) The commit description needs to be line wrapped at 75 characters
(Run scripts/checkpatch.pl on your path).

3) We need a Signed-off-by line.

4) The patch cannot apply.  Use `git diff /arch/mips/kernel/vpe.c` to
generate the patch.  (There are other ways to generate patches.  Read
Documentation/process/submitting-patches.rst or google for how to send
a first kernel patch).

regards,
dan carpenter

Patch

diff --git a/arch/mips/kernel/vpe.c_org b/arch/mips/kernel/vpe.c
index d0d832a..bd1f826 100644
--- a/arch/mips/kernel/vpe.c_org
+++ b/arch/mips/kernel/vpe.c
@@ -871,7 +871,7 @@  static ssize_t vpe_write(struct file *file, co  nst char __user *buffer,
        if (v == NULL)
                return -ENODEV;

-       if ((count + v->len) > v->plen) {
+       if ((count + v->len) > v->plen || count + v->len > v->len)   {
                pr_warn("VPE loader: elf size too big. Perhaps str  ip unneeded symbols\n");
                return -ENOMEM;
        }