linux-kernel - Re: [PATCH RFC] media: videobuf2: fix the length check for mmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 22 Apr 2013 11:03:02 +0530
From:	Prabhakar Lad <prabhakar.csengg@...il.com>
To:	Mauro Carvalho Chehab <mchehab@...hat.com>
Cc:	LMML <linux-media@...r.kernel.org>,
	DLOS <davinci-linux-open-source@...ux.davincidsp.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Hans Verkuil <hans.verkuil@...co.com>,
	Pawel Osciak <pawel@...iak.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Seung-Woo Kim <sw0312.kim@...sung.com>
Subject: Re: [PATCH RFC] media: videobuf2: fix the length check for mmap

On Fri, Apr 19, 2013 at 4:48 PM, Mauro Carvalho Chehab
<mchehab@...hat.com> wrote:
> Em Fri, 19 Apr 2013 15:16:56 +0530
> Prabhakar lad <prabhakar.csengg@...il.com> escreveu:
>
>> From: Lad, Prabhakar <prabhakar.csengg@...il.com>
>>
>> From commit 068a0df76023926af958a336a78bef60468d2033
>> "[media] media: vb2: add length check for mmap"
>> patch verifies that the mmap() size requested by userspace
>> doesn't exceed the buffer size.
>>
>> As the mmap() size is rounded up to the next page boundary
>> the check will fail for buffer sizes that are not multiple
>> of the page size.
>>
>> This patch fixes the check by aligning the buffer size to page
>> size during the check. Alongside fixes the vmalloc allocator
>> to round up the size.
>>
>> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@...il.com>
>> Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>
>> Cc: Marek Szyprowski <m.szyprowski@...sung.com>
>> Cc: Seung-Woo Kim <sw0312.kim@...sung.com>
>> Cc: Hans Verkuil <hans.verkuil@...co.com>
>> Cc: Mauro Carvalho Chehab <mchehab@...hat.com>
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c    |    2 +-
>>  drivers/media/v4l2-core/videobuf2-vmalloc.c |    2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>> index 58c1744..223fcd4 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -1886,7 +1886,7 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma)
>>
>>       vb = q->bufs[buffer];
>>
>> -     if (vb->v4l2_planes[plane].length < (vma->vm_end - vma->vm_start)) {
>> +     if (PAGE_ALIGN(vb->v4l2_planes[plane].length) < (vma->vm_end - vma->vm_start)) {
>>               dprintk(1, "Invalid length\n");
>>               return -EINVAL;
>>       }
>
> That is tricky, as it assumes that vb->v4l2_planes[plane].length was round
> up to PAGE_SIZE at each memops driver, but the vb2 core doesn't enforce it.
>
> IMO, it would be cleaner to round vb->v4l2_planes[plane].length up
> at VB2 core, before calling the memops alloc functions at the drivers.
>
> Also, VB2 is already complex enough to put it there without proper
> comments (and there's a minor codingstyle issue there: line is bigger
> than 80 cols).
>
>> diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c b/drivers/media/v4l2-core/videobuf2-vmalloc.c
>> index 313d977..bf3b95c 100644
>> --- a/drivers/media/v4l2-core/videobuf2-vmalloc.c
>> +++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c
>> @@ -44,7 +44,7 @@ static void *vb2_vmalloc_alloc(void *alloc_ctx, unsigned long size, gfp_t gfp_fl
>>               return NULL;
>>
>>       buf->size = size;
>> -     buf->vaddr = vmalloc_user(buf->size);
>> +     buf->vaddr = vmalloc_user(PAGE_ALIGN(buf->size));
>
> See? You needed to put an alignment here as well, not because vmalloc
> needs it, but because this is needed by VB2 core.
>
> Also, on the other drivers, buf->size is stored page aligned, while
> here, you're doing different, without any documented reason for doing
> that, instead of doing the same as on the other memops drivers.
>
> That mistake reflects, for example, when the driver prints the failure:
>
>         if (!buf->vaddr) {
>                 pr_debug("vmalloc of size %ld failed\n", buf->size);
>
> as it will show a different size than what you actually required.
> As those memory starving errors can also produce a dump at the mm
> core, the size there won't match the size on the above printed message.
>
> Also, it is a very bad idea to delegate the core's requirement of
> do page alignment from the core to the memops drivers, as other
> patches may change the logic there, or a new memops could be added,
> and the same problem will hit again (and unnoticed, as the check
> routine do page alignments).
>
>>       buf->handler.refcount = &buf->refcount;
>>       buf->handler.put = vb2_vmalloc_put;
>>       buf->handler.arg = buf;
>
> IMO, a cleaner version would be the following (untested) code.
>
> -
>
> [media] videobuf2: fix the length check for mmap
>
> Memory maps typically require that the buffer size to be page
> aligned. Currently, two memops drivers do such alignment
> internally, but videobuf-vmalloc doesn't.
>
> Also, the buffer overflow check doesn't take it into account.
>
> So, instead of doing it at each memops driver, enforce it at
> VB2 core.
>
> Reported-by: Prabhakar lad <prabhakar.csengg@...il.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@...hat.com>
>
Reported-and-tested-by: Lad, Prabhakar <prabhakar.csengg@...il.com>

Regards,
--Prabhakar
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/