Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650588 > unrolled thread

Re: [PATCH] drm/rockchip: Don't allow zero sized gem buffer

Started bySean Paul <seanpaul@chromium.org>
First post2017-05-25 17:40 +0200
Last post2017-05-26 16:00 +0200
Articles 5 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] drm/rockchip: Don't allow zero sized gem buffer Sean Paul <seanpaul@chromium.org> - 2017-05-25 17:40 +0200
    Re: [PATCH] drm/rockchip: Don't allow zero sized gem buffer jeffy <jeffy.chen@rock-chips.com> - 2017-05-26 04:40 +0200
      Re: [PATCH] drm/rockchip: Don't allow zero sized gem buffer Christoph Hellwig <hch@infradead.org> - 2017-05-26 08:00 +0200
        Re: [PATCH] drm/rockchip: Don't allow zero sized gem buffer Daniel Vetter <daniel@ffwll.ch> - 2017-05-26 09:00 +0200
      Re: [PATCH] drm/rockchip: Don't allow zero sized gem buffer Sean Paul <seanpaul@chromium.org> - 2017-05-26 16:00 +0200

#1650588 — Re: [PATCH] drm/rockchip: Don't allow zero sized gem buffer

FromSean Paul <seanpaul@chromium.org>
Date2017-05-25 17:40 +0200
SubjectRe: [PATCH] drm/rockchip: Don't allow zero sized gem buffer
Message-ID<tL31M-1xu-15@gated-at.bofh.it>
On Tue, May 23, 2017 at 02:39:43PM +0800, Jeffy Chen wrote:
> The system would crash when trying to alloc zero sized gem buffer:
> [    6.712435] Unable to handle kernel NULL pointer dereference at virtual address 00000010 <--ZERO_SIZE_PTR
> ...
> [    6.757502] PC is at sg_alloc_table_from_pages+0x170/0x1ec

It's unfortunate that you didn't include the entire stack trace. From code
inspection, it seems like the 0 size comes from the fb_probe path? Is there
somewhere in the helpers that you could check the mode is sane so all drivers
can benefit?

Sean

> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> index df9e570..8917922 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> @@ -315,6 +315,11 @@ struct rockchip_gem_object *
>  	struct drm_gem_object *obj;
>  	int ret;
>  
> +	if (!size) {
> +		DRM_ERROR("gem buffer size is zero\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	size = round_up(size, PAGE_SIZE);
>  
>  	rk_obj = kzalloc(sizeof(*rk_obj), GFP_KERNEL);
> -- 
> 2.1.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

[toc] | [next] | [standalone]

#1651043

Fromjeffy <jeffy.chen@rock-chips.com>
Date2017-05-26 04:40 +0200
Message-ID<tLdku-8nJ-1@gated-at.bofh.it>
In reply to#1650588
Hi sean,

On 05/25/2017 11:30 PM, Sean Paul wrote:
> On Tue, May 23, 2017 at 02:39:43PM +0800, Jeffy Chen wrote:
>> The system would crash when trying to alloc zero sized gem buffer:
>> [    6.712435] Unable to handle kernel NULL pointer dereference at virtual address 00000010 <--ZERO_SIZE_PTR
>> ...
>> [    6.757502] PC is at sg_alloc_table_from_pages+0x170/0x1ec
>
> It's unfortunate that you didn't include the entire stack trace. From code
> inspection, it seems like the 0 size comes from the fb_probe path? Is there
> somewhere in the helpers that you could check the mode is sane so all drivers
> can benefit?

hmm, sorry, i was testing it on chromeos 4.4 kernel, it turns out that 
we have a custom ioctl for userspace to create gem buffer(the same as 
exynos drm), which might get the the 0 size.

but on upstream kernel, it could only be called by dump_create, and the 
drm_mode_create_dumb_ioctl already did the size check.

will resent this patch, and rewrite the commit message, thanx.

>
> Sean
>
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> ---
>>
>>   drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>> index df9e570..8917922 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
>> @@ -315,6 +315,11 @@ struct rockchip_gem_object *
>>   	struct drm_gem_object *obj;
>>   	int ret;
>>
>> +	if (!size) {
>> +		DRM_ERROR("gem buffer size is zero\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>>   	size = round_up(size, PAGE_SIZE);
>>
>>   	rk_obj = kzalloc(sizeof(*rk_obj), GFP_KERNEL);
>> --
>> 2.1.4
>>
>

[toc] | [prev] | [next] | [standalone]

#1651093

FromChristoph Hellwig <hch@infradead.org>
Date2017-05-26 08:00 +0200
Message-ID<tLgs2-1UV-13@gated-at.bofh.it>
In reply to#1651043
On Fri, May 26, 2017 at 10:30:09AM +0800, jeffy wrote:
> Hi sean,
> 
> On 05/25/2017 11:30 PM, Sean Paul wrote:
> > On Tue, May 23, 2017 at 02:39:43PM +0800, Jeffy Chen wrote:
> > > The system would crash when trying to alloc zero sized gem buffer:
> > > [    6.712435] Unable to handle kernel NULL pointer dereference at virtual address 00000010 <--ZERO_SIZE_PTR
> > > ...
> > > [    6.757502] PC is at sg_alloc_table_from_pages+0x170/0x1ec
> > 
> > It's unfortunate that you didn't include the entire stack trace. From code
> > inspection, it seems like the 0 size comes from the fb_probe path? Is there
> > somewhere in the helpers that you could check the mode is sane so all drivers
> > can benefit?
> 
> hmm, sorry, i was testing it on chromeos 4.4 kernel, it turns out that we
> have a custom ioctl for userspace to create gem buffer(the same as exynos
> drm), which might get the the 0 size.
> 
> but on upstream kernel, it could only be called by dump_create, and the
> drm_mode_create_dumb_ioctl already did the size check.
> 
> will resent this patch, and rewrite the commit message, thanx.

That suggests that this patch isn't needed at all.

[toc] | [prev] | [next] | [standalone]

#1651110

FromDaniel Vetter <daniel@ffwll.ch>
Date2017-05-26 09:00 +0200
Message-ID<tLho5-2vy-3@gated-at.bofh.it>
In reply to#1651093
On Fri, May 26, 2017 at 7:52 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, May 26, 2017 at 10:30:09AM +0800, jeffy wrote:
>> Hi sean,
>>
>> On 05/25/2017 11:30 PM, Sean Paul wrote:
>> > On Tue, May 23, 2017 at 02:39:43PM +0800, Jeffy Chen wrote:
>> > > The system would crash when trying to alloc zero sized gem buffer:
>> > > [    6.712435] Unable to handle kernel NULL pointer dereference at virtual address 00000010 <--ZERO_SIZE_PTR
>> > > ...
>> > > [    6.757502] PC is at sg_alloc_table_from_pages+0x170/0x1ec
>> >
>> > It's unfortunate that you didn't include the entire stack trace. From code
>> > inspection, it seems like the 0 size comes from the fb_probe path? Is there
>> > somewhere in the helpers that you could check the mode is sane so all drivers
>> > can benefit?
>>
>> hmm, sorry, i was testing it on chromeos 4.4 kernel, it turns out that we
>> have a custom ioctl for userspace to create gem buffer(the same as exynos
>> drm), which might get the the 0 size.
>>
>> but on upstream kernel, it could only be called by dump_create, and the
>> drm_mode_create_dumb_ioctl already did the size check.
>>
>> will resent this patch, and rewrite the commit message, thanx.
>
> That suggests that this patch isn't needed at all.

Yes, not needed for upstream. But next time around pls include the
entire backtrace (or at least the relevant parts), not just the last
line, so that we can figure this out directly.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1651401

FromSean Paul <seanpaul@chromium.org>
Date2017-05-26 16:00 +0200
Message-ID<tLnWy-6BD-9@gated-at.bofh.it>
In reply to#1651043
On Fri, May 26, 2017 at 10:30:09AM +0800, jeffy wrote:
> Hi sean,
> 
> On 05/25/2017 11:30 PM, Sean Paul wrote:
> > On Tue, May 23, 2017 at 02:39:43PM +0800, Jeffy Chen wrote:
> > > The system would crash when trying to alloc zero sized gem buffer:
> > > [    6.712435] Unable to handle kernel NULL pointer dereference at virtual address 00000010 <--ZERO_SIZE_PTR
> > > ...
> > > [    6.757502] PC is at sg_alloc_table_from_pages+0x170/0x1ec
> > 
> > It's unfortunate that you didn't include the entire stack trace. From code
> > inspection, it seems like the 0 size comes from the fb_probe path? Is there
> > somewhere in the helpers that you could check the mode is sane so all drivers
> > can benefit?
> 
> hmm, sorry, i was testing it on chromeos 4.4 kernel, it turns out that we
> have a custom ioctl for userspace to create gem buffer(the same as exynos
> drm), which might get the the 0 size.
> 
> but on upstream kernel, it could only be called by dump_create, and the
> drm_mode_create_dumb_ioctl already did the size check.

Ah, ok. In that case, fix the custom ioctl such that it ensures we never call
this function with size == 0, and upload it downstream with a CHROMIUM prefix.

Sean


> 
> will resent this patch, and rewrite the commit message, thanx.
> 
> > 
> > Sean
> > 
> > > 
> > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > > ---
> > > 
> > >   drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> > > index df9e570..8917922 100644
> > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> > > @@ -315,6 +315,11 @@ struct rockchip_gem_object *
> > >   	struct drm_gem_object *obj;
> > >   	int ret;
> > > 
> > > +	if (!size) {
> > > +		DRM_ERROR("gem buffer size is zero\n");
> > > +		return ERR_PTR(-EINVAL);
> > > +	}
> > > +
> > >   	size = round_up(size, PAGE_SIZE);
> > > 
> > >   	rk_obj = kzalloc(sizeof(*rk_obj), GFP_KERNEL);
> > > --
> > > 2.1.4
> > > 
> > 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web