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@xxxxxx> 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 > -- Ondrej Zary