diff mbox series

[v2] fbmon: prevent division by zero in fb_videomode_from_videomode()

Message ID 20240305135150.23240-1-r.smirnov@omp.ru (mailing list archive)
State Superseded
Headers show
Series [v2] fbmon: prevent division by zero in fb_videomode_from_videomode() | expand

Commit Message

Roman Smirnov March 5, 2024, 1:51 p.m. UTC 
The expression htotal * vtotal can have a zero value on
overflow. It is necessary to prevent division by zero like in
fb_var_to_videomode().

Found by Linux Verification Center (linuxtesting.org) with Svace.

Signed-off-by: Roman Smirnov <r.smirnov@omp.ru>
Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
 V1 -> V2: Replaced the code of the first version with a check.

 drivers/video/fbdev/core/fbmon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Helge Deller March 15, 2024, 8:44 a.m. UTC | #1 
On 3/5/24 14:51, Roman Smirnov wrote:
> The expression htotal * vtotal can have a zero value on
> overflow.

I'm not sure if thos always results in zero in kernel on overflow.
Might be architecture-depended too, but let's assume it
can become zero, ....

> It is necessary to prevent division by zero like in
> fb_var_to_videomode().
>
> Found by Linux Verification Center (linuxtesting.org) with Svace.
>
> Signed-off-by: Roman Smirnov <r.smirnov@omp.ru>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
>   V1 -> V2: Replaced the code of the first version with a check.
>
>   drivers/video/fbdev/core/fbmon.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/core/fbmon.c b/drivers/video/fbdev/core/fbmon.c
> index 79e5bfbdd34c..b137590386da 100644
> --- a/drivers/video/fbdev/core/fbmon.c
> +++ b/drivers/video/fbdev/core/fbmon.c
> @@ -1344,7 +1344,7 @@ int fb_videomode_from_videomode(const struct videomode *vm,
>   	vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
>   		 vm->vsync_len;
>   	/* prevent division by zero */
> -	if (htotal && vtotal) {
> +	if (htotal && vtotal && (vm->pixelclock / htotal >= vtotal)) {

why don't you then simply check for
	if .. ((htotal * vtotal) == 0) ...
instead?

Helge

>   		fbmode->refresh = vm->pixelclock / (htotal * vtotal);
>   	/* a mode must have htotal and vtotal != 0 or it is invalid */
>   	} else {
Roman Smirnov March 18, 2024, 8:11 a.m. UTC | #2 
On Fri, 15 Mar 2024 09:44:08 +0100 Helge Deller wrote:
> On 3/5/24 14:51, Roman Smirnov wrote:
> > The expression htotal * vtotal can have a zero value on
> > overflow.
> 
> I'm not sure if thos always results in zero in kernel on overflow.
> Might be architecture-depended too, but let's assume it
> can become zero, ....
> 
> > It is necessary to prevent division by zero like in
> > fb_var_to_videomode().
> >
> > Found by Linux Verification Center (linuxtesting.org) with Svace.
> >
> > Signed-off-by: Roman Smirnov <r.smirnov@omp.ru>
> > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> > ---
> >   V1 -> V2: Replaced the code of the first version with a check.
> >
> >   drivers/video/fbdev/core/fbmon.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/video/fbdev/core/fbmon.c b/drivers/video/fbdev/core/fbmon.c
> > index 79e5bfbdd34c..b137590386da 100644
> > --- a/drivers/video/fbdev/core/fbmon.c
> > +++ b/drivers/video/fbdev/core/fbmon.c
> > @@ -1344,7 +1344,7 @@ int fb_videomode_from_videomode(const struct videomode *vm,
> >        vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
> >                 vm->vsync_len;
> >        /* prevent division by zero */
> > -     if (htotal && vtotal) {
> > +     if (htotal && vtotal && (vm->pixelclock / htotal >= vtotal)) {
> 
> why don't you then simply check for
>         if .. ((htotal * vtotal) == 0) ...
> instead?
> 
> Helge

Thomas Zimmermann from the previous discussion said:

On Tue, 5 Mar 2024 11:18:05 +0100 Thomas Zimmerman wrote:
> Maybe use
>
>    if (htotal && vtotal && (vm->pixelclock / htotal >= vtotal))
>
> for the test. That rules out overflowing multiplication and sets
> refresh to 0 in such cases.

This prevents overflow, which is also a problematic case.
Helge Deller March 18, 2024, 7:15 p.m. UTC | #3 
On 3/18/24 09:11, Roman Smirnov wrote:
> On Fri, 15 Mar 2024 09:44:08 +0100 Helge Deller wrote:
>> On 3/5/24 14:51, Roman Smirnov wrote:
>>> The expression htotal * vtotal can have a zero value on
>>> overflow.
>>
>> I'm not sure if those always results in zero in kernel on overflow.
>> Might be architecture-depended too, but let's assume it
>> can become zero, ....
>>
>>> It is necessary to prevent division by zero like in
>>> fb_var_to_videomode().
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with Svace.
>>>
>>> Signed-off-by: Roman Smirnov <r.smirnov@omp.ru>
>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>> ---
>>>    V1 -> V2: Replaced the code of the first version with a check.
>>>
>>>    drivers/video/fbdev/core/fbmon.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/video/fbdev/core/fbmon.c b/drivers/video/fbdev/core/fbmon.c
>>> index 79e5bfbdd34c..b137590386da 100644
>>> --- a/drivers/video/fbdev/core/fbmon.c
>>> +++ b/drivers/video/fbdev/core/fbmon.c
>>> @@ -1344,7 +1344,7 @@ int fb_videomode_from_videomode(const struct videomode *vm,
>>>         vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
>>>                  vm->vsync_len;
>>>         /* prevent division by zero */
>>> -     if (htotal && vtotal) {
>>> +     if (htotal && vtotal && (vm->pixelclock / htotal >= vtotal)) {
>>
>> why don't you then simply check for
>>          if .. ((htotal * vtotal) == 0) ...
>> instead?
>>
>> Helge
>
> Thomas Zimmermann from the previous discussion said:
>
> On Tue, 5 Mar 2024 11:18:05 +0100 Thomas Zimmerman wrote:
>> Maybe use
>>
>>     if (htotal && vtotal && (vm->pixelclock / htotal >= vtotal))
>>
>> for the test. That rules out overflowing multiplication and sets
>> refresh to 0 in such cases.
>
> This prevents overflow, which is also a problematic case.

I don't like adding another division here and I doubt we have
a problem with possible overflow.
So, I suggest to keep it simple, something like:
	...
	total = htotal * vtotal;
	if (total)
		fbmode->refresh = vm->pixelclock / total;
	else...
Helge
Roman Smirnov March 19, 2024, 8:12 a.m. UTC | #4 
On Mon, 18 Mar 2024 20:15:55 +0100 Helge Deller wrote:
> On 3/18/24 09:11, Roman Smirnov wrote:
> > On Fri, 15 Mar 2024 09:44:08 +0100 Helge Deller wrote:
> > > On 3/5/24 14:51, Roman Smirnov wrote:
> > > > The expression htotal * vtotal can have a zero value on
> > > > overflow.
> > >
> > > I'm not sure if those always results in zero in kernel on overflow.
> > > Might be architecture-depended too, but let's assume it
> > > can become zero, ....
> > >
> > > > It is necessary to prevent division by zero like in
> > > > fb_var_to_videomode().
> > > >
> > > > Found by Linux Verification Center (linuxtesting.org) with Svace.
> > > >
> > > > Signed-off-by: Roman Smirnov <r.smirnov@omp.ru>
> > > > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> > > > ---
> > > >    V1 -> V2: Replaced the code of the first version with a check.
> > > >
> > > >    drivers/video/fbdev/core/fbmon.c | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/video/fbdev/core/fbmon.c b/drivers/video/fbdev/core/fbmon.c
> > > > index 79e5bfbdd34c..b137590386da 100644
> > > > --- a/drivers/video/fbdev/core/fbmon.c
> > > > +++ b/drivers/video/fbdev/core/fbmon.c
> > > > @@ -1344,7 +1344,7 @@ int fb_videomode_from_videomode(const struct videomode *vm,
> > > >         vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
> > > >                  vm->vsync_len;
> > > >         /* prevent division by zero */
> > > > -     if (htotal && vtotal) {
> > > > +     if (htotal && vtotal && (vm->pixelclock / htotal >= vtotal)) {
> > >
> > > why don't you then simply check for
> > >          if .. ((htotal * vtotal) == 0) ...
> > > instead?
> > >
> > > Helge
> >
> > Thomas Zimmermann from the previous discussion said:
> > On Tue, 5 Mar 2024 11:18:05 +0100 Thomas Zimmerman wrote:
> > > Maybe use
> > >
> > >     if (htotal && vtotal && (vm->pixelclock / htotal >= vtotal))
> > >
> > > for the test. That rules out overflowing multiplication and sets
> > > refresh to 0 in such cases.
> >
> > This prevents overflow, which is also a problematic case.
>
> I don't like adding another division here and I doubt we have
> a problem with possible overflow.
> So, I suggest to keep it simple, something like:
>        ...
>        total = htotal * vtotal;
>        if (total)
>                fbmode->refresh = vm->pixelclock / total;
>        else...

Okay, I'll prepare a third version with that change:

    if (htotal && vtotal && (htotal * vtotal))

I think that will be enough.
Sergey Shtylyov March 19, 2024, 8:22 a.m. UTC | #5 
On 3/19/24 11:12 AM, Roman Smirnov wrote:
[...]

>>>> On 3/5/24 14:51, Roman Smirnov wrote:
>>>>> The expression htotal * vtotal can have a zero value on
>>>>> overflow.
>>>>
>>>> I'm not sure if those always results in zero in kernel on overflow.
>>>> Might be architecture-depended too, but let's assume it
>>>> can become zero, ....
>>>>
>>>>> It is necessary to prevent division by zero like in
>>>>> fb_var_to_videomode().
>>>>>
>>>>> Found by Linux Verification Center (linuxtesting.org) with Svace.
>>>>>
>>>>> Signed-off-by: Roman Smirnov <r.smirnov@omp.ru>
>>>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>>>> ---
>>>>>    V1 -> V2: Replaced the code of the first version with a check.
>>>>>
>>>>>    drivers/video/fbdev/core/fbmon.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/video/fbdev/core/fbmon.c b/drivers/video/fbdev/core/fbmon.c
>>>>> index 79e5bfbdd34c..b137590386da 100644
>>>>> --- a/drivers/video/fbdev/core/fbmon.c
>>>>> +++ b/drivers/video/fbdev/core/fbmon.c
>>>>> @@ -1344,7 +1344,7 @@ int fb_videomode_from_videomode(const struct videomode *vm,
>>>>>         vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
>>>>>                  vm->vsync_len;
>>>>>         /* prevent division by zero */
>>>>> -     if (htotal && vtotal) {
>>>>> +     if (htotal && vtotal && (vm->pixelclock / htotal >= vtotal)) {
>>>>
>>>> why don't you then simply check for
>>>>          if .. ((htotal * vtotal) == 0) ...
>>>> instead?
>>>>
>>>> Helge
>>>
>>> Thomas Zimmermann from the previous discussion said:
>>> On Tue, 5 Mar 2024 11:18:05 +0100 Thomas Zimmerman wrote:
>>>> Maybe use
>>>>
>>>>     if (htotal && vtotal && (vm->pixelclock / htotal >= vtotal))
>>>>
>>>> for the test. That rules out overflowing multiplication and sets
>>>> refresh to 0 in such cases.
>>>
>>> This prevents overflow, which is also a problematic case.
>>
>> I don't like adding another division here and I doubt we have
>> a problem with possible overflow.
>> So, I suggest to keep it simple, something like:
>>        ...
>>        total = htotal * vtotal;
>>        if (total)
>>                fbmode->refresh = vm->pixelclock / total;
>>        else...
> 
> Okay, I'll prepare a third version with that change:
> 
>     if (htotal && vtotal && (htotal * vtotal))

   I think the 1st 2 checks here are now redundant. Also, the inner
parens are not necessary...

> I think that will be enough.

   More than enough. :-)

MBR, Sergey
diff mbox series

Patch

diff --git a/drivers/video/fbdev/core/fbmon.c b/drivers/video/fbdev/core/fbmon.c
index 79e5bfbdd34c..b137590386da 100644
--- a/drivers/video/fbdev/core/fbmon.c
+++ b/drivers/video/fbdev/core/fbmon.c
@@ -1344,7 +1344,7 @@  int fb_videomode_from_videomode(const struct videomode *vm,
 	vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
 		 vm->vsync_len;
 	/* prevent division by zero */
-	if (htotal && vtotal) {
+	if (htotal && vtotal && (vm->pixelclock / htotal >= vtotal)) {
 		fbmode->refresh = vm->pixelclock / (htotal * vtotal);
 	/* a mode must have htotal and vtotal != 0 or it is invalid */
 	} else {