Hi,
I found a bug in the function i740fb_set_par().
When the user calls the ioctl system call without setting the value to 'var->pixclock', the driver will throw a divide error.
This bug occurs because the driver uses the value of 'var->pixclock' without checking it, as the following code snippet show:
if ((1000000 / var->pixclock) > DACSPEED8) { dev_err(info->device, "requested pixclock %i MHz out of range (max. %i MHz at 8bpp)\n", 1000000 / var->pixclock, DACSPEED8); return -EINVAL;x }
We can fix this by checking the value of 'var->pixclock' in the function i740fb_check_var() similar to commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09, or we should set the lowest supported value when this field is zero. I have no idea about which solution is better.
The following log reveals it:
divide error: 0000 [#1] PREEMPT SMP KASAN PTI RIP: 0010:i740fb_decode_var drivers/video/fbdev/i740fb.c:444 [inline] RIP: 0010:i740fb_set_par+0x272f/0x3bb0 drivers/video/fbdev/i740fb.c:739 Call Trace: <TASK> fb_set_var+0x604/0xeb0 drivers/video/fbdev/core/fbmem.c:1036 do_fb_ioctl+0x234/0x670 drivers/video/fbdev/core/fbmem.c:1112 fb_ioctl+0xdd/0x130 drivers/video/fbdev/core/fbmem.c:1191 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:874 [inline]
Regards, Zheyu Ma
On 4/3/22 13:26, Zheyu Ma wrote:
Hi,
I found a bug in the function i740fb_set_par().
Nice catch!
When the user calls the ioctl system call without setting the value to 'var->pixclock', the driver will throw a divide error.
This bug occurs because the driver uses the value of 'var->pixclock' without checking it, as the following code snippet show:
if ((1000000 / var->pixclock) > DACSPEED8) { dev_err(info->device, "requested pixclock %i MHz out of range (max. %i MHz at 8bpp)\n", 1000000 / var->pixclock, DACSPEED8); return -EINVAL;x }
We can fix this by checking the value of 'var->pixclock' in the function i740fb_check_var() similar to commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09, or we should set the lowest supported value when this field is zero. I have no idea about which solution is better.
Me neither. I think a solution like commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09 is sufficient.
Note that i740fb_set_par() is called in i740fb_resume() as well. Since this doesn't comes form userspace I think adding a check for the return value there isn't necessary.
Would you mind sending a patch like b36b242d4b8ea178f7fd038965e3cac7f30c3f09 ?
Helge
The following log reveals it:
divide error: 0000 [#1] PREEMPT SMP KASAN PTI RIP: 0010:i740fb_decode_var drivers/video/fbdev/i740fb.c:444 [inline] RIP: 0010:i740fb_set_par+0x272f/0x3bb0 drivers/video/fbdev/i740fb.c:739 Call Trace:
<TASK> fb_set_var+0x604/0xeb0 drivers/video/fbdev/core/fbmem.c:1036 do_fb_ioctl+0x234/0x670 drivers/video/fbdev/core/fbmem.c:1112 fb_ioctl+0xdd/0x130 drivers/video/fbdev/core/fbmem.c:1191 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:874 [inline]
Regards, Zheyu Ma
On Sun, Apr 3, 2022, 23:02 Helge Deller deller@gmx.de wrote:
On 4/3/22 13:26, Zheyu Ma wrote:
Hi,
I found a bug in the function i740fb_set_par().
Nice catch!
When the user calls the ioctl system call without setting the value to 'var->pixclock', the driver will throw a divide error.
This bug occurs because the driver uses the value of 'var->pixclock' without checking it, as the following code snippet show:
if ((1000000 / var->pixclock) > DACSPEED8) { dev_err(info->device, "requested pixclock %i MHz out of range (max. %i MHz at 8bpp)\n", 1000000 / var->pixclock, DACSPEED8); return -EINVAL;x }
We can fix this by checking the value of 'var->pixclock' in the function i740fb_check_var() similar to commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09, or we should set the lowest supported value when this field is zero. I have no idea about which solution is better.
Me neither. I think a solution like commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09 is sufficient.
Note that i740fb_set_par() is called in i740fb_resume() as well. Since this doesn't comes form userspace I think adding a check for the return value there isn't necessary.
Would you mind sending a patch like b36b242d4b8ea178f7fd038965e3cac7f30c3f09 ?
OK, and I also found similar bugs in other framebuffer drivers, I will send a patch set for them soon.
Zheyu Ma
Hi Helge,
On Sun, Apr 3, 2022 at 5:41 PM Helge Deller deller@gmx.de wrote:
On 4/3/22 13:26, Zheyu Ma wrote:
I found a bug in the function i740fb_set_par().
Nice catch!
When the user calls the ioctl system call without setting the value to 'var->pixclock', the driver will throw a divide error.
This bug occurs because the driver uses the value of 'var->pixclock' without checking it, as the following code snippet show:
if ((1000000 / var->pixclock) > DACSPEED8) { dev_err(info->device, "requested pixclock %i MHz out of range (max. %i MHz at 8bpp)\n", 1000000 / var->pixclock, DACSPEED8); return -EINVAL;x }
We can fix this by checking the value of 'var->pixclock' in the function i740fb_check_var() similar to commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09, or we should set the lowest supported value when this field is zero. I have no idea about which solution is better.
Me neither. I think a solution like commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09 is sufficient.
Note that i740fb_set_par() is called in i740fb_resume() as well. Since this doesn't comes form userspace I think adding a check for the return value there isn't necessary.
Would you mind sending a patch like b36b242d4b8ea178f7fd038965e3cac7f30c3f09 ?
When passed an invalid value, .check_var() is supposed to round up the invalid to a valid value, if possible.
Commit b36b242d4b8ea178 ("video: fbdev: asiliantfb: Error out if 'pixclock' equals zero") does not do that.
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hello Geert,
On 4/4/22 13:46, Geert Uytterhoeven wrote:
Hi Helge,
On Sun, Apr 3, 2022 at 5:41 PM Helge Deller deller@gmx.de wrote:
On 4/3/22 13:26, Zheyu Ma wrote:
I found a bug in the function i740fb_set_par().
Nice catch!
When the user calls the ioctl system call without setting the value to 'var->pixclock', the driver will throw a divide error.
This bug occurs because the driver uses the value of 'var->pixclock' without checking it, as the following code snippet show:
if ((1000000 / var->pixclock) > DACSPEED8) { dev_err(info->device, "requested pixclock %i MHz out of range (max. %i MHz at 8bpp)\n", 1000000 / var->pixclock, DACSPEED8); return -EINVAL;x }
We can fix this by checking the value of 'var->pixclock' in the function i740fb_check_var() similar to commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09, or we should set the lowest supported value when this field is zero. I have no idea about which solution is better.
Me neither. I think a solution like commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09 is sufficient.
Note that i740fb_set_par() is called in i740fb_resume() as well. Since this doesn't comes form userspace I think adding a check for the return value there isn't necessary.
Would you mind sending a patch like b36b242d4b8ea178f7fd038965e3cac7f30c3f09 ?
When passed an invalid value, .check_var() is supposed to round up the invalid to a valid value, if possible.
I don't disagree. The main problem probably is: what is the next valid value? This needs to be analyzed on a per-driver base and ideally tested. Right now a division-by-zero is tiggered which is probably more worse.
That said, currently I'd prefer to apply the zero-checks patches over any untested patches. It's easy to revert such checks if a better solution becomes available.
Thoughts?
Commit b36b242d4b8ea178 ("video: fbdev: asiliantfb: Error out if 'pixclock' equals zero") does not do that.
Helge
Hi Helge,
On Tue, Apr 5, 2022 at 8:34 AM Helge Deller deller@gmx.de wrote:
On 4/4/22 13:46, Geert Uytterhoeven wrote:
On Sun, Apr 3, 2022 at 5:41 PM Helge Deller deller@gmx.de wrote:
On 4/3/22 13:26, Zheyu Ma wrote:
I found a bug in the function i740fb_set_par().
Nice catch!
When the user calls the ioctl system call without setting the value to 'var->pixclock', the driver will throw a divide error.
This bug occurs because the driver uses the value of 'var->pixclock' without checking it, as the following code snippet show:
if ((1000000 / var->pixclock) > DACSPEED8) { dev_err(info->device, "requested pixclock %i MHz out of range (max. %i MHz at 8bpp)\n", 1000000 / var->pixclock, DACSPEED8); return -EINVAL;x }
We can fix this by checking the value of 'var->pixclock' in the function i740fb_check_var() similar to commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09, or we should set the lowest supported value when this field is zero. I have no idea about which solution is better.
Me neither. I think a solution like commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09 is sufficient.
Note that i740fb_set_par() is called in i740fb_resume() as well. Since this doesn't comes form userspace I think adding a check for the return value there isn't necessary.
Would you mind sending a patch like b36b242d4b8ea178f7fd038965e3cac7f30c3f09 ?
When passed an invalid value, .check_var() is supposed to round up the invalid to a valid value, if possible.
I don't disagree. The main problem probably is: what is the next valid value? This needs to be analyzed on a per-driver base and ideally tested. Right now a division-by-zero is tiggered which is probably more worse.
That said, currently I'd prefer to apply the zero-checks patches over any untested patches. It's easy to revert such checks if a better solution becomes available.
Thoughts?
Fair enough. And you're the maintainer ;-)
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tuesday 05 April 2022 08:33:57 Helge Deller wrote:
Hello Geert,
On 4/4/22 13:46, Geert Uytterhoeven wrote:
Hi Helge,
On Sun, Apr 3, 2022 at 5:41 PM Helge Deller deller@gmx.de wrote:
On 4/3/22 13:26, Zheyu Ma wrote:
I found a bug in the function i740fb_set_par().
Nice catch!
When the user calls the ioctl system call without setting the value to 'var->pixclock', the driver will throw a divide error.
This bug occurs because the driver uses the value of 'var->pixclock' without checking it, as the following code snippet show:
if ((1000000 / var->pixclock) > DACSPEED8) { dev_err(info->device, "requested pixclock %i MHz out of range (max. %i MHz at 8bpp)\n", 1000000 / var->pixclock, DACSPEED8); return -EINVAL;x }
We can fix this by checking the value of 'var->pixclock' in the function i740fb_check_var() similar to commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09, or we should set the lowest supported value when this field is zero. I have no idea about which solution is better.
Me neither. I think a solution like commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09 is sufficient.
Note that i740fb_set_par() is called in i740fb_resume() as well. Since this doesn't comes form userspace I think adding a check for the return value there isn't necessary.
Would you mind sending a patch like b36b242d4b8ea178f7fd038965e3cac7f30c3f09 ?
When passed an invalid value, .check_var() is supposed to round up the invalid to a valid value, if possible.
I don't disagree. The main problem probably is: what is the next valid value? This needs to be analyzed on a per-driver base and ideally tested. Right now a division-by-zero is tiggered which is probably more worse.
I still have an i740 card so I can test it.
That said, currently I'd prefer to apply the zero-checks patches over any untested patches. It's easy to revert such checks if a better solution becomes available.
Thoughts?
Commit b36b242d4b8ea178 ("video: fbdev: asiliantfb: Error out if 'pixclock' equals zero") does not do that.
Helge
On 4/5/22 19:46, Ondrej Zary wrote:
On Tuesday 05 April 2022 08:33:57 Helge Deller wrote:
Hello Geert,
On 4/4/22 13:46, Geert Uytterhoeven wrote:
Hi Helge,
On Sun, Apr 3, 2022 at 5:41 PM Helge Deller deller@gmx.de wrote:
On 4/3/22 13:26, Zheyu Ma wrote:
I found a bug in the function i740fb_set_par().
Nice catch!
When the user calls the ioctl system call without setting the value to 'var->pixclock', the driver will throw a divide error.
This bug occurs because the driver uses the value of 'var->pixclock' without checking it, as the following code snippet show:
if ((1000000 / var->pixclock) > DACSPEED8) { dev_err(info->device, "requested pixclock %i MHz out of range (max. %i MHz at 8bpp)\n", 1000000 / var->pixclock, DACSPEED8); return -EINVAL;x }
We can fix this by checking the value of 'var->pixclock' in the function i740fb_check_var() similar to commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09, or we should set the lowest supported value when this field is zero. I have no idea about which solution is better.
Me neither. I think a solution like commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09 is sufficient.
Note that i740fb_set_par() is called in i740fb_resume() as well. Since this doesn't comes form userspace I think adding a check for the return value there isn't necessary.
Would you mind sending a patch like b36b242d4b8ea178f7fd038965e3cac7f30c3f09 ?
When passed an invalid value, .check_var() is supposed to round up the invalid to a valid value, if possible.
I don't disagree. The main problem probably is: what is the next valid value? This needs to be analyzed on a per-driver base and ideally tested. Right now a division-by-zero is tiggered which is probably more worse.
I still have an i740 card so I can test it.
Good. Someone wants to come up with a proposed patch?
Helge
On Wed, Apr 6, 2022 at 2:23 AM Helge Deller deller@gmx.de wrote:
On 4/5/22 19:46, Ondrej Zary wrote:
On Tuesday 05 April 2022 08:33:57 Helge Deller wrote:
Hello Geert,
On 4/4/22 13:46, Geert Uytterhoeven wrote:
Hi Helge,
On Sun, Apr 3, 2022 at 5:41 PM Helge Deller deller@gmx.de wrote:
On 4/3/22 13:26, Zheyu Ma wrote:
I found a bug in the function i740fb_set_par().
Nice catch!
When the user calls the ioctl system call without setting the value to 'var->pixclock', the driver will throw a divide error.
This bug occurs because the driver uses the value of 'var->pixclock' without checking it, as the following code snippet show:
if ((1000000 / var->pixclock) > DACSPEED8) { dev_err(info->device, "requested pixclock %i MHz out of range (max. %i MHz at 8bpp)\n", 1000000 / var->pixclock, DACSPEED8); return -EINVAL;x }
We can fix this by checking the value of 'var->pixclock' in the function i740fb_check_var() similar to commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09, or we should set the lowest supported value when this field is zero. I have no idea about which solution is better.
Me neither. I think a solution like commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09 is sufficient.
Note that i740fb_set_par() is called in i740fb_resume() as well. Since this doesn't comes form userspace I think adding a check for the return value there isn't necessary.
Would you mind sending a patch like b36b242d4b8ea178f7fd038965e3cac7f30c3f09 ?
When passed an invalid value, .check_var() is supposed to round up the invalid to a valid value, if possible.
I don't disagree. The main problem probably is: what is the next valid value? This needs to be analyzed on a per-driver base and ideally tested. Right now a division-by-zero is tiggered which is probably more worse.
I still have an i740 card so I can test it.
Good. Someone wants to come up with a proposed patch?
I have submitted patches for the i740fb driver and other drivers which have similar bugs as follows: https://lore.kernel.org/all/20220404084723.79089-1-zheyuma97@gmail.com/
Zheyu Ma
On 4/6/22 03:24, Zheyu Ma wrote:
On Wed, Apr 6, 2022 at 2:23 AM Helge Deller deller@gmx.de wrote:
On 4/5/22 19:46, Ondrej Zary wrote:
On Tuesday 05 April 2022 08:33:57 Helge Deller wrote:
Hello Geert,
On 4/4/22 13:46, Geert Uytterhoeven wrote:
Hi Helge,
On Sun, Apr 3, 2022 at 5:41 PM Helge Deller deller@gmx.de wrote:
On 4/3/22 13:26, Zheyu Ma wrote: > I found a bug in the function i740fb_set_par().
Nice catch!
> When the user calls the ioctl system call without setting the value to > 'var->pixclock', the driver will throw a divide error. > > This bug occurs because the driver uses the value of 'var->pixclock' > without checking it, as the following code snippet show: > > if ((1000000 / var->pixclock) > DACSPEED8) { > dev_err(info->device, "requested pixclock %i MHz out of range > (max. %i MHz at 8bpp)\n", > 1000000 / var->pixclock, DACSPEED8); > return -EINVAL;x > } > > We can fix this by checking the value of 'var->pixclock' in the > function i740fb_check_var() similar to commit > b36b242d4b8ea178f7fd038965e3cac7f30c3f09, or we should set the lowest > supported value when this field is zero. > I have no idea about which solution is better.
Me neither. I think a solution like commit b36b242d4b8ea178f7fd038965e3cac7f30c3f09 is sufficient.
Note that i740fb_set_par() is called in i740fb_resume() as well. Since this doesn't comes form userspace I think adding a check for the return value there isn't necessary.
Would you mind sending a patch like b36b242d4b8ea178f7fd038965e3cac7f30c3f09 ?
When passed an invalid value, .check_var() is supposed to round up the invalid to a valid value, if possible.
I don't disagree. The main problem probably is: what is the next valid value? This needs to be analyzed on a per-driver base and ideally tested. Right now a division-by-zero is tiggered which is probably more worse.
I still have an i740 card so I can test it.
Good. Someone wants to come up with a proposed patch?
I have submitted patches for the i740fb driver and other drivers which have similar bugs as follows: https://lore.kernel.org/all/20220404084723.79089-1-zheyuma97@gmail.com/
Yes, I know. But Ondrej offered to test a patch which would round an invalid pixclock up instead of just returning EINVAL (which is what your patch does). So, if someone comes up with such a patch it'd be the preferred solution.
Helge
dri-devel@lists.freedesktop.org
-
Geert Uytterhoeven -
Helge Deller -
Ondrej Zary -
Zheyu Ma