Hi Alex,
On 6/12/25 11:29, Geert Uytterhoeven wrote:
On Wed, 11 Jun 2025 at 18:12, Alex Guo <[email protected]> wrote:
On Sat, 7 Jun 2025 at 22:14, Alex Guo <[email protected]> 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 <[email protected]>
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/
