| Message ID | 20220714141705.2375-1-sohu0106@126.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | MIPS: vpe: fix integer overflow in vpe_write() | expand |
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
[ 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
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; }
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(-)