2023-03-07 13:11:51

by Wei Chen

Subject: [PATCH] fbdev: tgafb: Fix potential divide by zero

fb_set_var would by called when user invokes ioctl with cmd
FBIOPUT_VSCREENINFO. User-provided data would finally reach
tgafb_check_var. In case var->pixclock is assigned to zero,
divide by zero would occur when checking whether reciprocal
of var->pixclock is too high.

Similar crashes have happened in other fbdev drivers. There
is no check and modification on var->pixclock along the call
chain to tgafb_check_var. We believe it could also be triggered
in driver tgafb from user site.

Signed-off-by: harperchen <[email protected]>
---
drivers/video/fbdev/tgafb.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/video/fbdev/tgafb.c b/drivers/video/fbdev/tgafb.c
index 14d37c49633c..b44004880f0d 100644
--- a/drivers/video/fbdev/tgafb.c
+++ b/drivers/video/fbdev/tgafb.c
@@ -173,6 +173,9 @@ tgafb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
{
struct tga_par *par = (struct tga_par *)info->par;

+ if (!var->pixclock)
+ return -EINVAL;
+
if (par->tga_type == TGA_TYPE_8PLANE) {
if (var->bits_per_pixel != 8)
return -EINVAL;
--
2.25.1

2023-03-08 22:06:10

by Helge Deller

Subject: Re: [PATCH] fbdev: tgafb: Fix potential divide by zero

On 3/7/23 14:08, harperchen wrote:
> fb_set_var would by called when user invokes ioctl with cmd
> FBIOPUT_VSCREENINFO. User-provided data would finally reach
> tgafb_check_var. In case var->pixclock is assigned to zero,
> divide by zero would occur when checking whether reciprocal
> of var->pixclock is too high.
>
> Similar crashes have happened in other fbdev drivers. There
> is no check and modification on var->pixclock along the call
> chain to tgafb_check_var. We believe it could also be triggered
> in driver tgafb from user site.
>
> Signed-off-by: harperchen <[email protected]>

Could you provide a real name?
Otherwise applied to fbdev git tree.

Thanks!
Helge

> ---
> drivers/video/fbdev/tgafb.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/video/fbdev/tgafb.c b/drivers/video/fbdev/tgafb.c
> index 14d37c49633c..b44004880f0d 100644
> --- a/drivers/video/fbdev/tgafb.c
> +++ b/drivers/video/fbdev/tgafb.c
> @@ -173,6 +173,9 @@ tgafb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
> {
> struct tga_par *par = (struct tga_par *)info->par;
>
> + if (!var->pixclock)
> + return -EINVAL;
> +
> if (par->tga_type == TGA_TYPE_8PLANE) {
> if (var->bits_per_pixel != 8)
> return -EINVAL;

2023-03-09 06:12:00

by Wei Chen

Subject: Re: [PATCH] fbdev: tgafb: Fix potential divide by zero

Dear Helge,

Thank you for the kind words. My real name is Wei Chen.

Please apply this patch to fbdev git tree if it is correct.

Best,
Wei

On Thu, 9 Mar 2023 at 06:05, Helge Deller <[email protected]> wrote:
>
> On 3/7/23 14:08, harperchen wrote:
> > fb_set_var would by called when user invokes ioctl with cmd
> > FBIOPUT_VSCREENINFO. User-provided data would finally reach
> > tgafb_check_var. In case var->pixclock is assigned to zero,
> > divide by zero would occur when checking whether reciprocal
> > of var->pixclock is too high.
> >
> > Similar crashes have happened in other fbdev drivers. There
> > is no check and modification on var->pixclock along the call
> > chain to tgafb_check_var. We believe it could also be triggered
> > in driver tgafb from user site.
> >
> > Signed-off-by: harperchen <[email protected]>
>
> Could you provide a real name?
> Otherwise applied to fbdev git tree.
>
> Thanks!
> Helge
>
> > ---
> > drivers/video/fbdev/tgafb.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/video/fbdev/tgafb.c b/drivers/video/fbdev/tgafb.c
> > index 14d37c49633c..b44004880f0d 100644
> > --- a/drivers/video/fbdev/tgafb.c
> > +++ b/drivers/video/fbdev/tgafb.c
> > @@ -173,6 +173,9 @@ tgafb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
> > {
> > struct tga_par *par = (struct tga_par *)info->par;
> >
> > + if (!var->pixclock)
> > + return -EINVAL;
> > +
> > if (par->tga_type == TGA_TYPE_8PLANE) {
> > if (var->bits_per_pixel != 8)
> > return -EINVAL;
>

2023-03-09 07:55:56

by Jani Nikula

Subject: Re: [PATCH] fbdev: tgafb: Fix potential divide by zero

On Wed, 08 Mar 2023, Helge Deller <[email protected]> wrote:
> On 3/7/23 14:08, harperchen wrote:
>> fb_set_var would by called when user invokes ioctl with cmd
>> FBIOPUT_VSCREENINFO. User-provided data would finally reach
>> tgafb_check_var. In case var->pixclock is assigned to zero,
>> divide by zero would occur when checking whether reciprocal
>> of var->pixclock is too high.
>>
>> Similar crashes have happened in other fbdev drivers. There
>> is no check and modification on var->pixclock along the call
>> chain to tgafb_check_var. We believe it could also be triggered
>> in driver tgafb from user site.
>>
>> Signed-off-by: harperchen <[email protected]>
>
> Could you provide a real name?
> Otherwise applied to fbdev git tree.

See commit d4563201f33a ("Documentation: simplify and clarify DCO
contribution example language").

BR,
Jani.


>
> Thanks!
> Helge
>
>> ---
>> drivers/video/fbdev/tgafb.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/video/fbdev/tgafb.c b/drivers/video/fbdev/tgafb.c
>> index 14d37c49633c..b44004880f0d 100644
>> --- a/drivers/video/fbdev/tgafb.c
>> +++ b/drivers/video/fbdev/tgafb.c
>> @@ -173,6 +173,9 @@ tgafb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
>> {
>> struct tga_par *par = (struct tga_par *)info->par;
>>
>> + if (!var->pixclock)
>> + return -EINVAL;
>> +
>> if (par->tga_type == TGA_TYPE_8PLANE) {
>> if (var->bits_per_pixel != 8)
>> return -EINVAL;
>

--
Jani Nikula, Intel Open Source Graphics Center

2023-03-09 08:20:32

by Helge Deller

Subject: Re: [PATCH] fbdev: tgafb: Fix potential divide by zero

On 3/9/23 08:53, Jani Nikula wrote:
> On Wed, 08 Mar 2023, Helge Deller <[email protected]> wrote:
>> On 3/7/23 14:08, harperchen wrote:
>>> fb_set_var would by called when user invokes ioctl with cmd
>>> FBIOPUT_VSCREENINFO. User-provided data would finally reach
>>> tgafb_check_var. In case var->pixclock is assigned to zero,
>>> divide by zero would occur when checking whether reciprocal
>>> of var->pixclock is too high.
>>>
>>> Similar crashes have happened in other fbdev drivers. There
>>> is no check and modification on var->pixclock along the call
>>> chain to tgafb_check_var. We believe it could also be triggered
>>> in driver tgafb from user site.
>>>
>>> Signed-off-by: harperchen <[email protected]>
>>
>> Could you provide a real name?
>> Otherwise applied to fbdev git tree.
>
> See commit d4563201f33a ("Documentation: simplify and clarify DCO
> contribution example language").

Nice. Thanks for that link!
Btw, I did applied that patch yesterday to my tree with just the nickname,
but of course I do prefer real names which is why I asked.

Helge