fbdev: tgafb: Fix potential divide by zero

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

Posted by harperchen 3 years, 1 month ago

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 <harperchen1110@gmail.com>
---
 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

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

Posted by Helge Deller 3 years, 1 month ago

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 <harperchen1110@gmail.com>

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;

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

Posted by Jani Nikula 3 years, 1 month ago

On Wed, 08 Mar 2023, Helge Deller <deller@gmx.de> 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 <harperchen1110@gmail.com>
>
> 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

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

Posted by Helge Deller 3 years, 1 month ago

On 3/9/23 08:53, Jani Nikula wrote:
> On Wed, 08 Mar 2023, Helge Deller <deller@gmx.de> 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 <harperchen1110@gmail.com>
>>
>> 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

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

Posted by Wei Chen 3 years, 1 month ago

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 <deller@gmx.de> 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 <harperchen1110@gmail.com>
>
> 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;
>