fbdev: pm3fb: Fix potential divide by zero

[PATCH] fbdev: pm3fb: Fix potential divide by zero

Posted by Alex Guo 6 months, 2 weeks ago

variable var->pixclock can be set by user. In case it equals to
 zero, divide by zero would occur in pm3fb_check_var. Similar
crashes have happened in other fbdev drivers. There is no check
and modification on var->pixclock along the call chain to
pm3fb_check_var. So we fix this by checking whether 'pixclock'
is zero.

Similar commit: commit 16844e58704 ("video: fbdev: tridentfb:
Error out if 'pixclock' equals zero")

Signed-off-by: Alex Guo <alexguo1023@gmail.com>
---
 drivers/video/fbdev/pm3fb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/video/fbdev/pm3fb.c b/drivers/video/fbdev/pm3fb.c
index 6e55e42514d6..d9b3f1937ce6 100644
--- a/drivers/video/fbdev/pm3fb.c
+++ b/drivers/video/fbdev/pm3fb.c
@@ -998,6 +998,9 @@ static int pm3fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
 		return -EINVAL;
 	}
 
+	if (!var->pixclock)
+		return -EINVAL;
+		
 	if (PICOS2KHZ(var->pixclock) > PM3_MAX_PIXCLOCK) {
 		DPRINTK("pixclock too high (%ldKHz)\n",
 			PICOS2KHZ(var->pixclock));
-- 
2.34.1

Re: [PATCH] fbdev: pm3fb: Fix potential divide by zero

Posted by Geert Uytterhoeven 6 months, 1 week ago

Hi Alex,

On Sat, 7 Jun 2025 at 22:14, Alex Guo <alexguo1023@gmail.com> wrote:
> variable var->pixclock can be set by user. In case it equals to
>  zero, divide by zero would occur in pm3fb_check_var. Similar
> crashes have happened in other fbdev drivers. There is no check
> and modification on var->pixclock along the call chain to
> pm3fb_check_var. So we fix this by checking whether 'pixclock'
> is zero.
>
> Similar commit: commit 16844e58704 ("video: fbdev: tridentfb:
> Error out if 'pixclock' equals zero")
>
> Signed-off-by: Alex Guo <alexguo1023@gmail.com>

Thanks for your patch, which is now commit 59d1fc7b3e1ae9d4
("fbdev: pm3fb: fix potential divide by zero") in fbdev/for-next.

> --- a/drivers/video/fbdev/pm3fb.c
> +++ b/drivers/video/fbdev/pm3fb.c
> @@ -998,6 +998,9 @@ static int pm3fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
>                 return -EINVAL;
>         }
>
> +       if (!var->pixclock)
> +               return -EINVAL;

While this fixes the crash, this is correct behavior for an fbdev driver.
When a value is invalid, it should be rounded up to a valid value instead,
if possible.

> +
>         if (PICOS2KHZ(var->pixclock) > PM3_MAX_PIXCLOCK) {
>                 DPRINTK("pixclock too high (%ldKHz)\n",
>                         PICOS2KHZ(var->pixclock));

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

Re: [PATCH] fbdev: pm3fb: Fix potential divide by zero

Posted by Alex Guo 6 months, 1 week ago

Hi Greet,

Thanks for your confirmation and suggestions.

I added this patch based on existing checks on var->pixclock in other drivers, such as savagefb_check_var, nvidiafb_check_var, etc.
Are you suggesting that it is better to replace an invalid value (var->pixclock == 0) with a default valid value, instead of returning -EINVAL? If so, could you advise what a suitable default value would be for this case?

Actually, I have found a few similar issues in other functions as well. I would like to make sure I am addressing them in the correct way.

Best,
Alex

Re: [PATCH] fbdev: pm3fb: Fix potential divide by zero

Posted by Geert Uytterhoeven 6 months, 1 week ago

Hi Alex,

On Wed, 11 Jun 2025 at 18:12, Alex Guo <alexguo1023@gmail.com> wrote:
> > On Sat, 7 Jun 2025 at 22:14, Alex Guo <alexguo1023@gmail.com> wrote:
> > > variable var->pixclock can be set by user. In case it equals to
> > >  zero, divide by zero would occur in pm3fb_check_var. Similar
> > > crashes have happened in other fbdev drivers. There is no check
> > > and modification on var->pixclock along the call chain to
> > > pm3fb_check_var. So we fix this by checking whether 'pixclock'
> > > is zero.
> > >
> > > Similar commit: commit 16844e58704 ("video: fbdev: tridentfb:
> > > Error out if 'pixclock' equals zero")
> > >
> > > Signed-off-by: Alex Guo <alexguo1023@gmail.com>
> >
> > Thanks for your patch, which is now commit 59d1fc7b3e1ae9d4
> > ("fbdev: pm3fb: fix potential divide by zero") in fbdev/for-next.
> >
> > > --- a/drivers/video/fbdev/pm3fb.c
> > > +++ b/drivers/video/fbdev/pm3fb.c
> > > @@ -998,6 +998,9 @@ static int pm3fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
> > >                 return -EINVAL;
> > >         }
> > >
> > > +       if (!var->pixclock)
> > > +               return -EINVAL;
> >
> > While this fixes the crash, this is correct behavior for an fbdev driver.
> > When a value is invalid, it should be rounded up to a valid value instead,
> > if possible.
>
> Thanks for your confirmation and suggestions.
>
> I added this patch based on existing checks on var->pixclock in other drivers, such as savagefb_check_var, nvidiafb_check_var, etc.
> Are you suggesting that it is better to replace an invalid value (var->pixclock == 0) with a default valid value, instead of returning -EINVAL?

Indeed.

> If so, could you advise what a suitable default value would be for this case?

The answer is hidden in the existing check below:

> > > +
> > >         if (PICOS2KHZ(var->pixclock) > PM3_MAX_PIXCLOCK) {
> > >                 DPRINTK("pixclock too high (%ldKHz)\n",
> > >                         PICOS2KHZ(var->pixclock));
> > >                 return -EINVAL;
> > >         }

It can be replaced by:

    if (var->pixclock <= KHZ2PICOS(PM3_MAX_PIXCLOCK))
            var->pixclock = KHZ2PICOS(PM3_MAX_PIXCLOCK) + 1;

The "+ 1" is needed because of rounding.

> Actually, I have found a few similar issues in other functions as well. I would like to make sure I am addressing them in the correct way.

That would be great. Thanks!

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

Re: [PATCH] fbdev: pm3fb: Fix potential divide by zero

Posted by Helge Deller 4 months, 3 weeks ago

Hi Alex,

On 6/12/25 11:29, Geert Uytterhoeven wrote:
> On Wed, 11 Jun 2025 at 18:12, Alex Guo <alexguo1023@gmail.com> wrote:
>>> On Sat, 7 Jun 2025 at 22:14, Alex Guo <alexguo1023@gmail.com> wrote:
>>>> variable var->pixclock can be set by user. In case it equals to
>>>>   zero, divide by zero would occur in pm3fb_check_var. Similar
>>>> crashes have happened in other fbdev drivers. There is no check
>>>> and modification on var->pixclock along the call chain to
>>>> pm3fb_check_var. So we fix this by checking whether 'pixclock'
>>>> is zero.
>>>>
>>>> Similar commit: commit 16844e58704 ("video: fbdev: tridentfb:
>>>> Error out if 'pixclock' equals zero")
>>>>
>>>> Signed-off-by: Alex Guo <alexguo1023@gmail.com>
>>>
>>> Thanks for your patch, which is now commit 59d1fc7b3e1ae9d4
>>> ("fbdev: pm3fb: fix potential divide by zero") in fbdev/for-next.
>>>
>>>> --- a/drivers/video/fbdev/pm3fb.c
>>>> +++ b/drivers/video/fbdev/pm3fb.c
>>>> @@ -998,6 +998,9 @@ static int pm3fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
>>>>                  return -EINVAL;
>>>>          }
>>>>
>>>> +       if (!var->pixclock)
>>>> +               return -EINVAL;
>>>
>>> While this fixes the crash, this is correct behavior for an fbdev driver.
>>> When a value is invalid, it should be rounded up to a valid value instead,
>>> if possible.
>>
>> Thanks for your confirmation and suggestions.
>>
>> I added this patch based on existing checks on var->pixclock in other drivers, such as savagefb_check_var, nvidiafb_check_var, etc.
>> Are you suggesting that it is better to replace an invalid value (var->pixclock == 0) with a default valid value, instead of returning -EINVAL?
> 
> Indeed.
> 
>> If so, could you advise what a suitable default value would be for this case?
> 
> The answer is hidden in the existing check below:
> 
>>>> +
>>>>          if (PICOS2KHZ(var->pixclock) > PM3_MAX_PIXCLOCK) {
>>>>                  DPRINTK("pixclock too high (%ldKHz)\n",
>>>>                          PICOS2KHZ(var->pixclock));
>>>>                  return -EINVAL;
>>>>          }
> 
> It can be replaced by:
> 
>      if (var->pixclock <= KHZ2PICOS(PM3_MAX_PIXCLOCK))
>              var->pixclock = KHZ2PICOS(PM3_MAX_PIXCLOCK) + 1;
> 
> The "+ 1" is needed because of rounding.

You sent a whole bunch of patches [1] which check pixclock against
zero, but you don't set the default value as Geert pointed out
above. Can you maybe revise your patches accordingly?

Helge

[1] https://patchwork.kernel.org/project/linux-fbdev/list/

Re: [PATCH] fbdev: pm3fb: Fix potential divide by zero

Posted by Helge Deller 6 months, 1 week ago

On 6/7/25 21:49, Alex Guo wrote:
> variable var->pixclock can be set by user. In case it equals to
>   zero, divide by zero would occur in pm3fb_check_var. Similar
> crashes have happened in other fbdev drivers. There is no check
> and modification on var->pixclock along the call chain to
> pm3fb_check_var. So we fix this by checking whether 'pixclock'
> is zero.
> 
> Similar commit: commit 16844e58704 ("video: fbdev: tridentfb:
> Error out if 'pixclock' equals zero")
> 
> Signed-off-by: Alex Guo <alexguo1023@gmail.com>
> ---
>   drivers/video/fbdev/pm3fb.c | 3 +++
>   1 file changed, 3 insertions(+)

applied.

Thanks!
Helge