[PATCH media-staging regression fix] media: videobuf2-dma-sg: Fix buf->vb NULL pointer dereference

[PATCH media-staging regression fix] media: videobuf2-dma-sg: Fix buf->vb NULL pointer dereference

[Hans de Goede]

Commit a4b83deb3e76 ("media: videobuf2: rework vb2_mem_ops API")
added a new vb member to struct vb2_dma_sg_buf, but it only added
code setting this to the vb2_dma_sg_alloc() function and not to the
vb2_dma_sg_get_userptr() and vb2_dma_sg_attach_dmabuf() which also
create vb2_dma_sg_buf objects.

This is causing a crash due to a NULL pointer deref when using
libcamera on devices with an Intel IPU3 (qcam app).

Fix these crashes by assigning buf->vb in the other 2 functions too,
note libcamera tests the vb2_dma_sg_get_userptr() path, the change
to the vb2_dma_sg_attach_dmabuf() path is untested.

Fixes: a4b83deb3e76 ("media: videobuf2: rework vb2_mem_ops API")
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index 33ee63a99139..0452ed9fac95 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -241,6 +241,7 @@ static void *vb2_dma_sg_get_userptr(struct vb2_buffer *vb, struct device *dev,
 	buf->offset = vaddr & ~PAGE_MASK;
 	buf->size = size;
 	buf->dma_sgt = &buf->sg_table;
+	buf->vb = vb;
 	vec = vb2_create_framevec(vaddr, size);
 	if (IS_ERR(vec))
 		goto userptr_fail_pfnvec;
@@ -642,6 +643,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct vb2_buffer *vb, struct device *dev,
 	buf->dma_dir = vb->vb2_queue->dma_dir;
 	buf->size = size;
 	buf->db_attach = dba;
+	buf->vb = vb;
 
 	return buf;
 }
-- 
2.31.1

Re: [PATCH media-staging regression fix] media: videobuf2-dma-sg: Fix buf->vb NULL pointer dereference

[Hans de Goede]

Hi,

On 11/1/21 15:53, Hans de Goede wrote:
> Commit a4b83deb3e76 ("media: videobuf2: rework vb2_mem_ops API")
> added a new vb member to struct vb2_dma_sg_buf, but it only added
> code setting this to the vb2_dma_sg_alloc() function and not to the
> vb2_dma_sg_get_userptr() and vb2_dma_sg_attach_dmabuf() which also
> create vb2_dma_sg_buf objects.
> 
> This is causing a crash due to a NULL pointer deref when using
> libcamera on devices with an Intel IPU3 (qcam app).
> 
> Fix these crashes by assigning buf->vb in the other 2 functions too,
> note libcamera tests the vb2_dma_sg_get_userptr() path, the change
> to the vb2_dma_sg_attach_dmabuf() path is untested.
> 
> Fixes: a4b83deb3e76 ("media: videobuf2: rework vb2_mem_ops API")
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Ping ? This is still an issue in the current media-staging tree.

Regards,

Hans


> ---
>  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> index 33ee63a99139..0452ed9fac95 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> @@ -241,6 +241,7 @@ static void *vb2_dma_sg_get_userptr(struct vb2_buffer *vb, struct device *dev,
>  	buf->offset = vaddr & ~PAGE_MASK;
>  	buf->size = size;
>  	buf->dma_sgt = &buf->sg_table;
> +	buf->vb = vb;
>  	vec = vb2_create_framevec(vaddr, size);
>  	if (IS_ERR(vec))
>  		goto userptr_fail_pfnvec;
> @@ -642,6 +643,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct vb2_buffer *vb, struct device *dev,
>  	buf->dma_dir = vb->vb2_queue->dma_dir;
>  	buf->size = size;
>  	buf->db_attach = dba;
> +	buf->vb = vb;
>  
>  	return buf;
>  }
> 

Re: [PATCH media-staging regression fix] media: videobuf2-dma-sg: Fix buf->vb NULL pointer dereference

[Tomasz Figa]

Hi Hans,

On Sat, Nov 6, 2021 at 9:39 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11/1/21 15:53, Hans de Goede wrote:
> > Commit a4b83deb3e76 ("media: videobuf2: rework vb2_mem_ops API")
> > added a new vb member to struct vb2_dma_sg_buf, but it only added
> > code setting this to the vb2_dma_sg_alloc() function and not to the
> > vb2_dma_sg_get_userptr() and vb2_dma_sg_attach_dmabuf() which also
> > create vb2_dma_sg_buf objects.
> >
> > This is causing a crash due to a NULL pointer deref when using
> > libcamera on devices with an Intel IPU3 (qcam app).
> >
> > Fix these crashes by assigning buf->vb in the other 2 functions too,
> > note libcamera tests the vb2_dma_sg_get_userptr() path, the change
> > to the vb2_dma_sg_attach_dmabuf() path is untested.
> >
> > Fixes: a4b83deb3e76 ("media: videobuf2: rework vb2_mem_ops API")
> > Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Ping ? This is still an issue in the current media-staging tree.

Uh, sorry, I thought this was already fixed by [1], but that was only
for the dma-contig allocator. Thanks for the patch.

Acked-by: Tomasz Figa <tfiga@chromium.org>

[1] https://patchwork.kernel.org/project/linux-media/patch/20210928034634.333785-1-senozhatsky@chromium.org/

Hans (V.), would you pick this fix, please?

Best regards,
Tomasz

>
> Regards,
>
> Hans
>
>
> > ---
> >  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > index 33ee63a99139..0452ed9fac95 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > @@ -241,6 +241,7 @@ static void *vb2_dma_sg_get_userptr(struct vb2_buffer *vb, struct device *dev,
> >       buf->offset = vaddr & ~PAGE_MASK;
> >       buf->size = size;
> >       buf->dma_sgt = &buf->sg_table;
> > +     buf->vb = vb;
> >       vec = vb2_create_framevec(vaddr, size);
> >       if (IS_ERR(vec))
> >               goto userptr_fail_pfnvec;
> > @@ -642,6 +643,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct vb2_buffer *vb, struct device *dev,
> >       buf->dma_dir = vb->vb2_queue->dma_dir;
> >       buf->size = size;
> >       buf->db_attach = dba;
> > +     buf->vb = vb;
> >
> >       return buf;
> >  }
> >
>

Re: [PATCH media-staging regression fix] media: videobuf2-dma-sg: Fix buf->vb NULL pointer dereference

[Hans Verkuil]

On 15/11/2021 09:54, Tomasz Figa wrote:
> Hi Hans,
> 
> On Sat, Nov 6, 2021 at 9:39 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 11/1/21 15:53, Hans de Goede wrote:
>>> Commit a4b83deb3e76 ("media: videobuf2: rework vb2_mem_ops API")
>>> added a new vb member to struct vb2_dma_sg_buf, but it only added
>>> code setting this to the vb2_dma_sg_alloc() function and not to the
>>> vb2_dma_sg_get_userptr() and vb2_dma_sg_attach_dmabuf() which also
>>> create vb2_dma_sg_buf objects.
>>>
>>> This is causing a crash due to a NULL pointer deref when using
>>> libcamera on devices with an Intel IPU3 (qcam app).
>>>
>>> Fix these crashes by assigning buf->vb in the other 2 functions too,
>>> note libcamera tests the vb2_dma_sg_get_userptr() path, the change
>>> to the vb2_dma_sg_attach_dmabuf() path is untested.
>>>
>>> Fixes: a4b83deb3e76 ("media: videobuf2: rework vb2_mem_ops API")
>>> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Ping ? This is still an issue in the current media-staging tree.
> 
> Uh, sorry, I thought this was already fixed by [1], but that was only
> for the dma-contig allocator. Thanks for the patch.
> 
> Acked-by: Tomasz Figa <tfiga@chromium.org>
> 
> [1] https://patchwork.kernel.org/project/linux-media/patch/20210928034634.333785-1-senozhatsky@chromium.org/
> 
> Hans (V.), would you pick this fix, please?

Yes, it's in a PR I posted for 5.16.

Regards,

	Hans

> 
> Best regards,
> Tomasz
> 
>>
>> Regards,
>>
>> Hans
>>
>>
>>> ---
>>>  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>>> index 33ee63a99139..0452ed9fac95 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>>> @@ -241,6 +241,7 @@ static void *vb2_dma_sg_get_userptr(struct vb2_buffer *vb, struct device *dev,
>>>       buf->offset = vaddr & ~PAGE_MASK;
>>>       buf->size = size;
>>>       buf->dma_sgt = &buf->sg_table;
>>> +     buf->vb = vb;
>>>       vec = vb2_create_framevec(vaddr, size);
>>>       if (IS_ERR(vec))
>>>               goto userptr_fail_pfnvec;
>>> @@ -642,6 +643,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct vb2_buffer *vb, struct device *dev,
>>>       buf->dma_dir = vb->vb2_queue->dma_dir;
>>>       buf->size = size;
>>>       buf->db_attach = dba;
>>> +     buf->vb = vb;
>>>
>>>       return buf;
>>>  }
>>>
>>