| Message ID | 20181116234227.27255-1-sudipm.mukherjee@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: videobuf2-core: fix use after free in vb2_mmap | expand |
Same patch was already posted and is waiting to be merged: https://patchwork.linuxtv.org/patch/52944/ Regards, Hans On 11/17/2018 12:42 AM, Sudip Mukherjee wrote: > When we are using __find_plane_by_offset() to find the matching plane > number and the buffer, the queue is not locked. So, after we have > calculated the buffer number and assigned the pointer to vb, it can get > freed. And if that happens then we get a use-after-free when we try to > use this for the mmap and get the following calltrace: > > [ 30.623259] Call Trace: > [ 30.623531] dump_stack+0x244/0x39d > [ 30.623914] ? dump_stack_print_info.cold.1+0x20/0x20 > [ 30.624439] ? printk+0xa7/0xcf > [ 30.624777] ? kmsg_dump_rewind_nolock+0xe4/0xe4 > [ 30.625265] print_address_description.cold.7+0x9/0x1ff > [ 30.625809] kasan_report.cold.8+0x242/0x309 > [ 30.626263] ? vb2_mmap+0x712/0x790 > [ 30.626637] __asan_report_load8_noabort+0x14/0x20 > [ 30.627201] vb2_mmap+0x712/0x790 > [ 30.627642] ? vb2_poll+0x1d0/0x1d0 > [ 30.628060] vb2_fop_mmap+0x4b/0x70 > [ 30.628458] v4l2_mmap+0x153/0x200 > [ 30.628929] mmap_region+0xe85/0x1cd0 > > Lock the queue before we start finding the matching plane and buffer so > that there is no chance of the memory being freed while we are about > to use it. > > Reported-by: syzbot+be93025dd45dccd8923c@syzkaller.appspotmail.com > Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com> > --- > drivers/media/common/videobuf2/videobuf2-core.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 975ff5669f72..a81320566e02 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -2125,9 +2125,12 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma) > /* > * Find the plane corresponding to the offset passed by userspace. > */ > + mutex_lock(&q->mmap_lock); > ret = __find_plane_by_offset(q, off, &buffer, &plane); > - if (ret) > + if (ret) { > + mutex_unlock(&q->mmap_lock); > return ret; > + } > > vb = q->bufs[buffer]; > > @@ -2138,12 +2141,12 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma) > */ > length = PAGE_ALIGN(vb->planes[plane].length); > if (length < (vma->vm_end - vma->vm_start)) { > + mutex_unlock(&q->mmap_lock); > dprintk(1, > "MMAP invalid, as it would overflow buffer length\n"); > return -EINVAL; > } > > - mutex_lock(&q->mmap_lock); > ret = call_memop(vb, mmap, vb->planes[plane].mem_priv, vma); > mutex_unlock(&q->mmap_lock); > if (ret) >
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 975ff5669f72..a81320566e02 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -2125,9 +2125,12 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma) /* * Find the plane corresponding to the offset passed by userspace. */ + mutex_lock(&q->mmap_lock); ret = __find_plane_by_offset(q, off, &buffer, &plane); - if (ret) + if (ret) { + mutex_unlock(&q->mmap_lock); return ret; + } vb = q->bufs[buffer]; @@ -2138,12 +2141,12 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma) */ length = PAGE_ALIGN(vb->planes[plane].length); if (length < (vma->vm_end - vma->vm_start)) { + mutex_unlock(&q->mmap_lock); dprintk(1, "MMAP invalid, as it would overflow buffer length\n"); return -EINVAL; } - mutex_lock(&q->mmap_lock); ret = call_memop(vb, mmap, vb->planes[plane].mem_priv, vma); mutex_unlock(&q->mmap_lock); if (ret)
When we are using __find_plane_by_offset() to find the matching plane number and the buffer, the queue is not locked. So, after we have calculated the buffer number and assigned the pointer to vb, it can get freed. And if that happens then we get a use-after-free when we try to use this for the mmap and get the following calltrace: [ 30.623259] Call Trace: [ 30.623531] dump_stack+0x244/0x39d [ 30.623914] ? dump_stack_print_info.cold.1+0x20/0x20 [ 30.624439] ? printk+0xa7/0xcf [ 30.624777] ? kmsg_dump_rewind_nolock+0xe4/0xe4 [ 30.625265] print_address_description.cold.7+0x9/0x1ff [ 30.625809] kasan_report.cold.8+0x242/0x309 [ 30.626263] ? vb2_mmap+0x712/0x790 [ 30.626637] __asan_report_load8_noabort+0x14/0x20 [ 30.627201] vb2_mmap+0x712/0x790 [ 30.627642] ? vb2_poll+0x1d0/0x1d0 [ 30.628060] vb2_fop_mmap+0x4b/0x70 [ 30.628458] v4l2_mmap+0x153/0x200 [ 30.628929] mmap_region+0xe85/0x1cd0 Lock the queue before we start finding the matching plane and buffer so that there is no chance of the memory being freed while we are about to use it. Reported-by: syzbot+be93025dd45dccd8923c@syzkaller.appspotmail.com Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com> --- drivers/media/common/videobuf2/videobuf2-core.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)