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

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

[Ning Qiang]

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)".

Signed-off-by: Ning Qiang <sohu0106@126.com>
---
 arch/mips/kernel/vpe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/kernel/vpe.c b/arch/mips/kernel/vpe.c
index 13294972707b..1529e755200d 100644
--- a/arch/mips/kernel/vpe.c
+++ b/arch/mips/kernel/vpe.c
@@ -849,7 +849,7 @@ static ssize_t vpe_write(struct file *file, const 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 strip unneeded symbols\n");
 		return -ENOMEM;
 	}
-- 
2.25.1

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

[Dan Carpenter]

On Thu, Jul 14, 2022 at 10:17:05PM +0800, Ning Qiang 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)".
> 
> Signed-off-by: Ning Qiang <sohu0106@126.com>
> ---

Thanks!

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

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

[Linus Torvalds]

[ This was in my spambox, it may have been marked as spam for others too ]

On Thu, Jul 14, 2022 at 7:17 AM Ning Qiang <sohu0106@126.com> 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).

We reject oversized IO requests to read/write, and clamp them at
MAX_RW_COUNT (which is basically INT_MAX rounded down to a page size).

Exactly to avoid issues like this.

So you shouldn't be able to pass in those kinds of counts, and it
won't be able to overflow the arithmetic unless 'v->len' can grow
boundlessly, which I don't think it can.

So I suspect this is not actually possible, but if you have a
test-case that shows me wrong, that woudl be interesing.

That said: that 'vpe_write()' thing is *odd*. Why is it using that
'v->len' thing, instead of using the passed-in offset?

So there is certainly somewhat strange code in there, and it would
probably be good to take a look at it. But being a very special MIPS
driver, I'm not sure how many people care...

               Linus