Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
On 7/23/20 4:21 PM, Greg Kroah-Hartman wrote:
> On Wed, Jul 22, 2020 at 10:07:06AM +0200, Daniel Vetter wrote:
>> On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
>> wrote:
>>>
>>> On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
>>>> On 2020/07/16 19:00, Daniel Vetter wrote:
>>>>> On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
>>>>>> On 2020/07/16 0:12, Dan Carpenter wrote:
>>>>>>> I've complained about integer overflows in fbdev for a long time...
>>>>>>>
>>>>>>> What I'd like to see is something like the following maybe. I don't
>>>>>>> know how to get the vc_data in fbmem.c so it doesn't include your checks
>>>>>>> for negative.
>>>>>>
>>>>>> Yes. Like I said "Thus, I consider that we need more sanity/constraints
>>>>>> checks." at
>>>>>> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329...@i-love.sakura.ne.jp/
>>>>>> ,
>>>>>> we want basic checks. That's a task for fbdev people who should be
>>>>>> familiar with
>>>>>> necessary constraints.
>>>>>
>>>>> I think the worldwide supply of people who understand fbdev and willing to
>>>>> work on it is roughly 0. So if someone wants to fix this mess properly
>>>>> (which likely means adding tons of over/underflow checks at entry points,
>>>>> since you're never going to catch the driver bugs, there's too many and
>>>>> not enough people who care) they need to fix this themselves.
>>>>
>>>> But I think we can enforce reasonable constraint which is much stricter
>>>> than Dan's basic_checks()
>>>> (which used INT_MAX). For example, do we need to accept var->{xres,yres}
>>>> >= 1048576, for
>>>> "32768 rows or cols" * "32 pixels per character" = 1045876 and
>>>> vc_do_resize() accepts only
>>>> rows and cols < 32768 ?
>>>>
>>>>>
>>>>> Just to avoid confusion here.
>>>>>
>>>>>> Anyway, my two patches are small and low cost; can we apply these
>>>>>> patches regardless
>>>>>> of basic checks?
>>>>>
>>>>> Which two patches where?
>>>>
>>>> [PATCH v3] vt: Reject zero-sized screen buffer size.
>>>> from
>>>> https://lkml.kernel.org/r/20200712111013.11881-1-penguin-ker...@i-love.sakura.ne.jp
>>>
>>> This is now in my tree.
>>>
>>>> [PATCH v2] fbdev: Detect integer underflow at "struct
>>>> fbcon_ops"->clear_margins.
>>>> from
>>>> https://lkml.kernel.org/r/20200715015102.3814-1-penguin-ker...@i-love.sakura.ne.jp
>>>
>>> That should be taken by the fbdev maintainer, but I can take it too if
>>> people want.
>>
>> Just missed this weeks pull request train and feeling like not worth
>> making this an exception (it's been broken forever after all), so
>> maybe best if you just add this to vt.
>>
>> Acked-by: Daniel Vetter
>>
>> Also this avoids the impression I know what's going on in fbdev code,
>> maybe with sufficient abandon from my side someone will pop up who
>> cares an fixes the bazillion of syzkaller issues we seem to have
>> around console/vt and everything related.
>
> Great, will go queue it up now, thanks!
Fine with me, thanks!
PS I'll later queue the patch from George to drm-misc-next (after
reading both fbdev patches in detail it seems that both are needed).
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R Institute Poland
Samsung Electronics
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
On Wed, Jul 22, 2020 at 10:07:06AM +0200, Daniel Vetter wrote:
> On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
> wrote:
> >
> > On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
> > > On 2020/07/16 19:00, Daniel Vetter wrote:
> > > > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> > > >> On 2020/07/16 0:12, Dan Carpenter wrote:
> > > >>> I've complained about integer overflows in fbdev for a long time...
> > > >>>
> > > >>> What I'd like to see is something like the following maybe. I don't
> > > >>> know how to get the vc_data in fbmem.c so it doesn't include your
> > > >>> checks
> > > >>> for negative.
> > > >>
> > > >> Yes. Like I said "Thus, I consider that we need more
> > > >> sanity/constraints checks." at
> > > >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329...@i-love.sakura.ne.jp/
> > > >> ,
> > > >> we want basic checks. That's a task for fbdev people who should be
> > > >> familiar with
> > > >> necessary constraints.
> > > >
> > > > I think the worldwide supply of people who understand fbdev and willing
> > > > to
> > > > work on it is roughly 0. So if someone wants to fix this mess properly
> > > > (which likely means adding tons of over/underflow checks at entry
> > > > points,
> > > > since you're never going to catch the driver bugs, there's too many and
> > > > not enough people who care) they need to fix this themselves.
> > >
> > > But I think we can enforce reasonable constraint which is much stricter
> > > than Dan's basic_checks()
> > > (which used INT_MAX). For example, do we need to accept var->{xres,yres}
> > > >= 1048576, for
> > > "32768 rows or cols" * "32 pixels per character" = 1045876 and
> > > vc_do_resize() accepts only
> > > rows and cols < 32768 ?
> > >
> > > >
> > > > Just to avoid confusion here.
> > > >
> > > >> Anyway, my two patches are small and low cost; can we apply these
> > > >> patches regardless
> > > >> of basic checks?
> > > >
> > > > Which two patches where?
> > >
> > > [PATCH v3] vt: Reject zero-sized screen buffer size.
> > > from
> > > https://lkml.kernel.org/r/20200712111013.11881-1-penguin-ker...@i-love.sakura.ne.jp
> >
> > This is now in my tree.
> >
> > > [PATCH v2] fbdev: Detect integer underflow at "struct
> > > fbcon_ops"->clear_margins.
> > > from
> > > https://lkml.kernel.org/r/20200715015102.3814-1-penguin-ker...@i-love.sakura.ne.jp
> >
> > That should be taken by the fbdev maintainer, but I can take it too if
> > people want.
>
> Just missed this weeks pull request train and feeling like not worth
> making this an exception (it's been broken forever after all), so
> maybe best if you just add this to vt.
>
> Acked-by: Daniel Vetter
>
> Also this avoids the impression I know what's going on in fbdev code,
> maybe with sufficient abandon from my side someone will pop up who
> cares an fixes the bazillion of syzkaller issues we seem to have
> around console/vt and everything related.
Great, will go queue it up now, thanks!
greg k-h
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
On Tue, Jul 21, 2020 at 6:08 PM Greg Kroah-Hartman
wrote:
>
> On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
> > On 2020/07/16 19:00, Daniel Vetter wrote:
> > > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> > >> On 2020/07/16 0:12, Dan Carpenter wrote:
> > >>> I've complained about integer overflows in fbdev for a long time...
> > >>>
> > >>> What I'd like to see is something like the following maybe. I don't
> > >>> know how to get the vc_data in fbmem.c so it doesn't include your checks
> > >>> for negative.
> > >>
> > >> Yes. Like I said "Thus, I consider that we need more sanity/constraints
> > >> checks." at
> > >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329...@i-love.sakura.ne.jp/
> > >> ,
> > >> we want basic checks. That's a task for fbdev people who should be
> > >> familiar with
> > >> necessary constraints.
> > >
> > > I think the worldwide supply of people who understand fbdev and willing to
> > > work on it is roughly 0. So if someone wants to fix this mess properly
> > > (which likely means adding tons of over/underflow checks at entry points,
> > > since you're never going to catch the driver bugs, there's too many and
> > > not enough people who care) they need to fix this themselves.
> >
> > But I think we can enforce reasonable constraint which is much stricter
> > than Dan's basic_checks()
> > (which used INT_MAX). For example, do we need to accept var->{xres,yres} >=
> > 1048576, for
> > "32768 rows or cols" * "32 pixels per character" = 1045876 and
> > vc_do_resize() accepts only
> > rows and cols < 32768 ?
> >
> > >
> > > Just to avoid confusion here.
> > >
> > >> Anyway, my two patches are small and low cost; can we apply these
> > >> patches regardless
> > >> of basic checks?
> > >
> > > Which two patches where?
> >
> > [PATCH v3] vt: Reject zero-sized screen buffer size.
> > from
> > https://lkml.kernel.org/r/20200712111013.11881-1-penguin-ker...@i-love.sakura.ne.jp
>
> This is now in my tree.
>
> > [PATCH v2] fbdev: Detect integer underflow at "struct
> > fbcon_ops"->clear_margins.
> > from
> > https://lkml.kernel.org/r/20200715015102.3814-1-penguin-ker...@i-love.sakura.ne.jp
>
> That should be taken by the fbdev maintainer, but I can take it too if
> people want.
Just missed this weeks pull request train and feeling like not worth
making this an exception (it's been broken forever after all), so
maybe best if you just add this to vt.
Acked-by: Daniel Vetter
Also this avoids the impression I know what's going on in fbdev code,
maybe with sufficient abandon from my side someone will pop up who
cares an fixes the bazillion of syzkaller issues we seem to have
around console/vt and everything related.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
On Thu, Jul 16, 2020 at 08:27:21PM +0900, Tetsuo Handa wrote:
> On 2020/07/16 19:00, Daniel Vetter wrote:
> > On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
> >> On 2020/07/16 0:12, Dan Carpenter wrote:
> >>> I've complained about integer overflows in fbdev for a long time...
> >>>
> >>> What I'd like to see is something like the following maybe. I don't
> >>> know how to get the vc_data in fbmem.c so it doesn't include your checks
> >>> for negative.
> >>
> >> Yes. Like I said "Thus, I consider that we need more sanity/constraints
> >> checks." at
> >> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329...@i-love.sakura.ne.jp/
> >> ,
> >> we want basic checks. That's a task for fbdev people who should be
> >> familiar with
> >> necessary constraints.
> >
> > I think the worldwide supply of people who understand fbdev and willing to
> > work on it is roughly 0. So if someone wants to fix this mess properly
> > (which likely means adding tons of over/underflow checks at entry points,
> > since you're never going to catch the driver bugs, there's too many and
> > not enough people who care) they need to fix this themselves.
>
> But I think we can enforce reasonable constraint which is much stricter than
> Dan's basic_checks()
> (which used INT_MAX). For example, do we need to accept var->{xres,yres} >=
> 1048576, for
> "32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize()
> accepts only
> rows and cols < 32768 ?
>
> >
> > Just to avoid confusion here.
> >
> >> Anyway, my two patches are small and low cost; can we apply these patches
> >> regardless
> >> of basic checks?
> >
> > Which two patches where?
>
> [PATCH v3] vt: Reject zero-sized screen buffer size.
> from
> https://lkml.kernel.org/r/20200712111013.11881-1-penguin-ker...@i-love.sakura.ne.jp
This is now in my tree.
> [PATCH v2] fbdev: Detect integer underflow at "struct
> fbcon_ops"->clear_margins.
> from
> https://lkml.kernel.org/r/20200715015102.3814-1-penguin-ker...@i-love.sakura.ne.jp
That should be taken by the fbdev maintainer, but I can take it too if
people want.
thanks,
greg k-h
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
On 2020/07/16 19:00, Daniel Vetter wrote:
> On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote:
>> On 2020/07/16 0:12, Dan Carpenter wrote:
>>> I've complained about integer overflows in fbdev for a long time...
>>>
>>> What I'd like to see is something like the following maybe. I don't
>>> know how to get the vc_data in fbmem.c so it doesn't include your checks
>>> for negative.
>>
>> Yes. Like I said "Thus, I consider that we need more sanity/constraints
>> checks." at
>> https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329...@i-love.sakura.ne.jp/
>> ,
>> we want basic checks. That's a task for fbdev people who should be familiar
>> with
>> necessary constraints.
>
> I think the worldwide supply of people who understand fbdev and willing to
> work on it is roughly 0. So if someone wants to fix this mess properly
> (which likely means adding tons of over/underflow checks at entry points,
> since you're never going to catch the driver bugs, there's too many and
> not enough people who care) they need to fix this themselves.
But I think we can enforce reasonable constraint which is much stricter than
Dan's basic_checks()
(which used INT_MAX). For example, do we need to accept var->{xres,yres} >=
1048576, for
"32768 rows or cols" * "32 pixels per character" = 1045876 and vc_do_resize()
accepts only
rows and cols < 32768 ?
>
> Just to avoid confusion here.
>
>> Anyway, my two patches are small and low cost; can we apply these patches
>> regardless
>> of basic checks?
>
> Which two patches where?
[PATCH v3] vt: Reject zero-sized screen buffer size.
from
https://lkml.kernel.org/r/20200712111013.11881-1-penguin-ker...@i-love.sakura.ne.jp
[PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
from
https://lkml.kernel.org/r/20200715015102.3814-1-penguin-ker...@i-love.sakura.ne.jp
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
On Thu, Jul 16, 2020 at 12:29:00AM +0900, Tetsuo Handa wrote: > On 2020/07/16 0:12, Dan Carpenter wrote: > > I've complained about integer overflows in fbdev for a long time... > > > > What I'd like to see is something like the following maybe. I don't > > know how to get the vc_data in fbmem.c so it doesn't include your checks > > for negative. > > Yes. Like I said "Thus, I consider that we need more sanity/constraints > checks." at > https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329...@i-love.sakura.ne.jp/ > , > we want basic checks. That's a task for fbdev people who should be familiar > with > necessary constraints. I think the worldwide supply of people who understand fbdev and willing to work on it is roughly 0. So if someone wants to fix this mess properly (which likely means adding tons of over/underflow checks at entry points, since you're never going to catch the driver bugs, there's too many and not enough people who care) they need to fix this themselves. Just to avoid confusion here. > Anyway, my two patches are small and low cost; can we apply these patches > regardless > of basic checks? Which two patches where? Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
On 2020/07/16 0:12, Dan Carpenter wrote: > I've complained about integer overflows in fbdev for a long time... > > What I'd like to see is something like the following maybe. I don't > know how to get the vc_data in fbmem.c so it doesn't include your checks > for negative. Yes. Like I said "Thus, I consider that we need more sanity/constraints checks." at https://lore.kernel.org/lkml/b1e7dd6a-fc22-bba8-0abb-d3e779329...@i-love.sakura.ne.jp/ , we want basic checks. That's a task for fbdev people who should be familiar with necessary constraints. Anyway, my two patches are small and low cost; can we apply these patches regardless of basic checks? ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
On 2020/07/15 18:48, Dan Carpenter wrote:
>> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct
>> fb_info *info,
>> region.color = color;
>> region.rop = ROP_COPY;
>>
>> -if (rw && !bottom_only) {
>> +if ((int) rw > 0 && !bottom_only) {
>> region.dx = info->var.xoffset + rs;
> ^^
>
> If you choose a very high positive "rw" then this addition can overflow.
> info->var.xoffset comes from the user and I don't think it's checked...
Well, I think it would be checked by "struct fb_ops"->check_var hook.
For example, vmw_fb_check_var() has
if ((var->xoffset + var->xres) > par->max_width ||
(var->yoffset + var->yres) > par->max_height) {
DRM_ERROR("Requested geom can not fit in framebuffer\n");
return -EINVAL;
}
check. Of course, there might be integer overflow in that check...
Having sanity check at caller of "struct fb_ops"->check_var might be nice.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
On 2020/07/15 20:17, Tetsuo Handa wrote:
> On 2020/07/15 18:48, Dan Carpenter wrote:
>>> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc,
>>> struct fb_info *info,
>>> region.color = color;
>>> region.rop = ROP_COPY;
>>>
>>> - if (rw && !bottom_only) {
>>> + if ((int) rw > 0 && !bottom_only) {
>>> region.dx = info->var.xoffset + rs;
>> ^^
>>
>> If you choose a very high positive "rw" then this addition can overflow.
>> info->var.xoffset comes from the user and I don't think it's checked...
>
> Well, I think it would be checked by "struct fb_ops"->check_var hook.
> For example, vmw_fb_check_var() has
>
> if ((var->xoffset + var->xres) > par->max_width ||
> (var->yoffset + var->yres) > par->max_height) {
> DRM_ERROR("Requested geom can not fit in framebuffer\n");
> return -EINVAL;
> }
>
> check. Of course, there might be integer overflow in that check...
> Having sanity check at caller of "struct fb_ops"->check_var might be nice.
>
Well, while
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
ioctl(fd, FBIOGET_VSCREENINFO, );
var.xres = var.yres = 4;
var.xoffset = 4294967292U;
ioctl(fd, FBIOPUT_VSCREENINFO, );
bypassed
(var->xoffset + var->xres) > par->max_width
check in vmw_fb_check_var(),
--
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c
@@ -216,6 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct
fb_info *info,
region.color = color;
region.rop = ROP_COPY;
+ printk(KERN_INFO "%s info->var.xoffset=%u rs=%u info->var.yoffset=%u
bs=%u\n", __func__, info->var.xoffset, rs, info->var.yoffset, bs);
if ((int) rw > 0 && !bottom_only) {
region.dx = info->var.xoffset + rs;
region.dy = 0;
--
says that info->var.xoffset does not come from the user.
--
bit_clear_margins info->var.xoffset=0 rs=1024 info->var.yoffset=0 bs=800
--
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
On Wed, Jul 15, 2020 at 11:02:58PM +0900, Tetsuo Handa wrote:
> On 2020/07/15 20:17, Tetsuo Handa wrote:
> > On 2020/07/15 18:48, Dan Carpenter wrote:
> >>> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc,
> >>> struct fb_info *info,
> >>> region.color = color;
> >>> region.rop = ROP_COPY;
> >>>
> >>> - if (rw && !bottom_only) {
> >>> + if ((int) rw > 0 && !bottom_only) {
> >>> region.dx = info->var.xoffset + rs;
> >> ^^
> >>
> >> If you choose a very high positive "rw" then this addition can overflow.
> >> info->var.xoffset comes from the user and I don't think it's checked...
> >
> > Well, I think it would be checked by "struct fb_ops"->check_var hook.
> > For example, vmw_fb_check_var() has
> >
> > if ((var->xoffset + var->xres) > par->max_width ||
> > (var->yoffset + var->yres) > par->max_height) {
> > DRM_ERROR("Requested geom can not fit in framebuffer\n");
> > return -EINVAL;
> > }
> >
> > check. Of course, there might be integer overflow in that check...
> > Having sanity check at caller of "struct fb_ops"->check_var might be nice.
> >
>
> Well, while
>
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> ioctl(fd, FBIOGET_VSCREENINFO, );
> var.xres = var.yres = 4;
> var.xoffset = 4294967292U;
> ioctl(fd, FBIOPUT_VSCREENINFO, );
>
> bypassed
>
> (var->xoffset + var->xres) > par->max_width
>
> check in vmw_fb_check_var(),
>
> --
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -216,6 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct
> fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> + printk(KERN_INFO "%s info->var.xoffset=%u rs=%u info->var.yoffset=%u
> bs=%u\n", __func__, info->var.xoffset, rs, info->var.yoffset, bs);
> if ((int) rw > 0 && !bottom_only) {
> region.dx = info->var.xoffset + rs;
> region.dy = 0;
> --
>
> says that info->var.xoffset does not come from the user.
>
> --
> bit_clear_margins info->var.xoffset=0 rs=1024 info->var.yoffset=0 bs=800
> --
In fb_set_var() we do:
drivers/video/fbdev/core/fbmem.c
1055 ret = info->fbops->fb_check_var(var, info);
1056
1057 if (ret)
1058 return ret;
1059
1060 if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW)
1061 return 0;
1062
1063 if (!basic_checks(var))
1064 return -EINVAL;
1065
1066 if (info->fbops->fb_get_caps) {
1067 ret = fb_check_caps(info, var, var->activate);
1068
1069 if (ret)
1070 return ret;
1071 }
1072
1073 old_var = info->var;
1074 info->var = *var;
This should set "info->var.offset".
1075
1076 if (info->fbops->fb_set_par) {
1077 ret = info->fbops->fb_set_par(info);
1078
1079 if (ret) {
1080 info->var = old_var;
1081 printk(KERN_WARNING "detected "
I've complained about integer overflows in fbdev for a long time...
What I'd like to see is something like the following maybe. I don't
know how to get the vc_data in fbmem.c so it doesn't include your checks
for negative.
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index caf817bcb05c..5c74181fea5d 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -934,6 +934,54 @@ fb_pan_display(struct fb_info *info, struct
fb_var_screeninfo *var)
}
EXPORT_SYMBOL(fb_pan_display);
+static bool basic_checks(struct fb_var_screeninfo *var)
+{
+ unsigned int v_margins, h_margins;
+
+ /* I think var->height and var->width == UINT_MAX means something. */
+
+ if (var->xres > INT_MAX ||
+ var->yres > INT_MAX ||
+ var->xres_virtual > INT_MAX ||
+ var->yres_virtual > INT_MAX ||
+ var->xoffset > INT_MAX ||
+ var->yoffset > INT_MAX ||
+ var->left_margin > INT_MAX ||
+ var->right_margin > INT_MAX ||
+ var->upper_margin > INT_MAX ||
+ var->lower_margin > INT_MAX ||
+ var->hsync_len > INT_MAX ||
+ var->vsync_len > INT_MAX)
+ return false;
+
+ if (var->bits_per_pixel > 128)
+ return false;
+ if (var->rotate > FB_ROTATE_CCW)
+ return false;
+
+ if (var->xoffset > INT_MAX - var->xres)
+ return false;
+ if (var->yoffset > INT_MAX - var->yres)
+ return false;
+
+ if (var->left_margin > INT_MAX - var->right_margin ||
+ var->upper_margin >
Re: [PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
On Wed, Jul 15, 2020 at 10:51:02AM +0900, Tetsuo Handa wrote:
> syzbot is reporting general protection fault in bitfill_aligned() [1]
> caused by integer underflow in bit_clear_margins(). The cause of this
> problem is when and how do_vc_resize() updates vc->vc_{cols,rows}.
>
> If vc_do_resize() fails (e.g. kzalloc() fails) when var.xres or var.yres
> is going to shrink, vc->vc_{cols,rows} will not be updated. This allows
> bit_clear_margins() to see info->var.xres < (vc->vc_cols * cw) or
> info->var.yres < (vc->vc_rows * ch). Unexpectedly large rw or bh will
> try to overrun the __iomem region and causes general protection fault.
>
> Also, vc_resize(vc, 0, 0) does not set vc->vc_{cols,rows} = 0 due to
>
> new_cols = (cols ? cols : vc->vc_cols);
> new_rows = (lines ? lines : vc->vc_rows);
>
> exception. Since cols and lines are calculated as
>
> cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
> rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
> cols /= vc->vc_font.width;
> rows /= vc->vc_font.height;
> vc_resize(vc, cols, rows);
>
> in fbcon_modechanged(), var.xres < vc->vc_font.width makes cols = 0
> and var.yres < vc->vc_font.height makes rows = 0. This means that
>
> const int fd = open("/dev/fb0", O_ACCMODE);
> struct fb_var_screeninfo var = { };
> ioctl(fd, FBIOGET_VSCREENINFO, );
> var.xres = var.yres = 1;
> ioctl(fd, FBIOPUT_VSCREENINFO, );
>
> easily reproduces integer underflow bug explained above.
>
> Of course, callers of vc_resize() are not handling vc_do_resize() failure
> is bad. But we can't avoid vc_resize(vc, 0, 0) which returns 0. Therefore,
> as a band-aid workaround, this patch checks integer underflow in
> "struct fbcon_ops"->clear_margins call, assuming that
> vc->vc_cols * vc->vc_font.width and vc->vc_rows * vc->vc_font.heigh do not
> cause integer overflow.
>
> [1]
> https://syzkaller.appspot.com/bug?id=a565882df74fa76f10d3a6fec4be31098dbb37c6
>
> Reported-and-tested-by: syzbot
>
> Signed-off-by: Tetsuo Handa
> ---
> drivers/video/fbdev/core/bitblit.c | 4 ++--
> drivers/video/fbdev/core/fbcon_ccw.c | 4 ++--
> drivers/video/fbdev/core/fbcon_cw.c | 4 ++--
> drivers/video/fbdev/core/fbcon_ud.c | 4 ++--
> 4 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/bitblit.c
> b/drivers/video/fbdev/core/bitblit.c
> index ca935c09a261..35ebeeccde4d 100644
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct
> fb_info *info,
> region.color = color;
> region.rop = ROP_COPY;
>
> - if (rw && !bottom_only) {
> + if ((int) rw > 0 && !bottom_only) {
> region.dx = info->var.xoffset + rs;
^^
If you choose a very high positive "rw" then this addition can overflow.
info->var.xoffset comes from the user and I don't think it's checked...
regards,
dan carpenter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins.
syzbot is reporting general protection fault in bitfill_aligned() [1]
caused by integer underflow in bit_clear_margins(). The cause of this
problem is when and how do_vc_resize() updates vc->vc_{cols,rows}.
If vc_do_resize() fails (e.g. kzalloc() fails) when var.xres or var.yres
is going to shrink, vc->vc_{cols,rows} will not be updated. This allows
bit_clear_margins() to see info->var.xres < (vc->vc_cols * cw) or
info->var.yres < (vc->vc_rows * ch). Unexpectedly large rw or bh will
try to overrun the __iomem region and causes general protection fault.
Also, vc_resize(vc, 0, 0) does not set vc->vc_{cols,rows} = 0 due to
new_cols = (cols ? cols : vc->vc_cols);
new_rows = (lines ? lines : vc->vc_rows);
exception. Since cols and lines are calculated as
cols = FBCON_SWAP(ops->rotate, info->var.xres, info->var.yres);
rows = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres);
cols /= vc->vc_font.width;
rows /= vc->vc_font.height;
vc_resize(vc, cols, rows);
in fbcon_modechanged(), var.xres < vc->vc_font.width makes cols = 0
and var.yres < vc->vc_font.height makes rows = 0. This means that
const int fd = open("/dev/fb0", O_ACCMODE);
struct fb_var_screeninfo var = { };
ioctl(fd, FBIOGET_VSCREENINFO, );
var.xres = var.yres = 1;
ioctl(fd, FBIOPUT_VSCREENINFO, );
easily reproduces integer underflow bug explained above.
Of course, callers of vc_resize() are not handling vc_do_resize() failure
is bad. But we can't avoid vc_resize(vc, 0, 0) which returns 0. Therefore,
as a band-aid workaround, this patch checks integer underflow in
"struct fbcon_ops"->clear_margins call, assuming that
vc->vc_cols * vc->vc_font.width and vc->vc_rows * vc->vc_font.heigh do not
cause integer overflow.
[1]
https://syzkaller.appspot.com/bug?id=a565882df74fa76f10d3a6fec4be31098dbb37c6
Reported-and-tested-by: syzbot
Signed-off-by: Tetsuo Handa
---
drivers/video/fbdev/core/bitblit.c | 4 ++--
drivers/video/fbdev/core/fbcon_ccw.c | 4 ++--
drivers/video/fbdev/core/fbcon_cw.c | 4 ++--
drivers/video/fbdev/core/fbcon_ud.c | 4 ++--
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/video/fbdev/core/bitblit.c
b/drivers/video/fbdev/core/bitblit.c
index ca935c09a261..35ebeeccde4d 100644
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c
@@ -216,7 +216,7 @@ static void bit_clear_margins(struct vc_data *vc, struct
fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = info->var.xoffset + rs;
region.dy = 0;
region.width = rw;
@@ -224,7 +224,7 @@ static void bit_clear_margins(struct vc_data *vc, struct
fb_info *info,
info->fbops->fb_fillrect(info, );
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset;
region.dy = info->var.yoffset + bs;
region.width = rs;
diff --git a/drivers/video/fbdev/core/fbcon_ccw.c
b/drivers/video/fbdev/core/fbcon_ccw.c
index dfa9a8aa4509..78f3a5621478 100644
--- a/drivers/video/fbdev/core/fbcon_ccw.c
+++ b/drivers/video/fbdev/core/fbcon_ccw.c
@@ -201,7 +201,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct
fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = 0;
region.dy = info->var.yoffset;
region.height = rw;
@@ -209,7 +209,7 @@ static void ccw_clear_margins(struct vc_data *vc, struct
fb_info *info,
info->fbops->fb_fillrect(info, );
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset + bs;
region.dy = 0;
region.height = info->var.yres_virtual;
diff --git a/drivers/video/fbdev/core/fbcon_cw.c
b/drivers/video/fbdev/core/fbcon_cw.c
index ce08251bfd38..fd098ff17574 100644
--- a/drivers/video/fbdev/core/fbcon_cw.c
+++ b/drivers/video/fbdev/core/fbcon_cw.c
@@ -184,7 +184,7 @@ static void cw_clear_margins(struct vc_data *vc, struct
fb_info *info,
region.color = color;
region.rop = ROP_COPY;
- if (rw && !bottom_only) {
+ if ((int) rw > 0 && !bottom_only) {
region.dx = 0;
region.dy = info->var.yoffset + rs;
region.height = rw;
@@ -192,7 +192,7 @@ static void cw_clear_margins(struct vc_data *vc, struct
fb_info *info,
info->fbops->fb_fillrect(info, );
}
- if (bh) {
+ if ((int) bh > 0) {
region.dx = info->var.xoffset;
region.dy = info->var.yoffset;
region.height = info->var.yres;
diff --git a/drivers/video/fbdev/core/fbcon_ud.c
b/drivers/video/fbdev/core/fbcon_ud.c
index 1936afc78fec..e165a3fad29a 100644
---
