[PATCH 0/1] fbdev: atyfb: Fix buffer overflow

[PATCH 0/1] fbdev: atyfb: Fix buffer overflow

[Denis Arefev]

The fixes I suggested are not the only ones.
There are more options for solving this problem.

1.Find datasheet for the chip, find out the register offset, calculate
   the address using the formula (4*Dword offset), add this value to
   the array lt_lcd_regs[] at index LCD_MISC_CNTL.

2. Delete this code completely, as the chip is very obsolete and 
   not applicable.

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

Denis Arefev (1):
  fbdev: atyfb: Fix buffer overflow

 drivers/video/fbdev/aty/atyfb_base.c | 4 ++++
 1 file changed, 4 insertions(+)

base-commit: 1a9239bb4253f9076b5b4b2a1a4e8d7defd77a95
-- 
2.43.0

[PATCH 1/1] fbdev: atyfb: Fix buffer overflow

[Denis Arefev]

The value LCD_MISC_CNTL is used in the 'aty_st_lcd()' function to
calculate an index for accessing an array element of size 9.
This may cause a buffer overflow.

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

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Denis Arefev <arefev@swemel.ru>
---
 drivers/video/fbdev/aty/atyfb_base.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
index 210fd3ac18a4..93eb5eb6042b 100644
--- a/drivers/video/fbdev/aty/atyfb_base.c
+++ b/drivers/video/fbdev/aty/atyfb_base.c
@@ -149,6 +149,8 @@ static const u32 lt_lcd_regs[] = {
 void aty_st_lcd(int index, u32 val, const struct atyfb_par *par)
 {
 	if (M64_HAS(LT_LCD_REGS)) {
+		if ((u32)index >= ARRAY_SIZE(lt_lcd_regs))
+			return;
 		aty_st_le32(lt_lcd_regs[index], val, par);
 	} else {
 		unsigned long temp;
@@ -164,6 +166,8 @@ void aty_st_lcd(int index, u32 val, const struct atyfb_par *par)
 u32 aty_ld_lcd(int index, const struct atyfb_par *par)
 {
 	if (M64_HAS(LT_LCD_REGS)) {
+		if ((u32)index >= ARRAY_SIZE(lt_lcd_regs))
+			return 0;
 		return aty_ld_le32(lt_lcd_regs[index], par);
 	} else {
 		unsigned long temp;
-- 
2.43.0

Re: [PATCH 1/1] fbdev: atyfb: Fix buffer overflow

[Jani Nikula]

On Thu, 27 Mar 2025, Denis Arefev <arefev@swemel.ru> wrote:
> The value LCD_MISC_CNTL is used in the 'aty_st_lcd()' function to
> calculate an index for accessing an array element of size 9.
> This may cause a buffer overflow.

The fix is to fix it, not silently brush it under the carpet.

BR,
Jani.

>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Denis Arefev <arefev@swemel.ru>
> ---
>  drivers/video/fbdev/aty/atyfb_base.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
> index 210fd3ac18a4..93eb5eb6042b 100644
> --- a/drivers/video/fbdev/aty/atyfb_base.c
> +++ b/drivers/video/fbdev/aty/atyfb_base.c
> @@ -149,6 +149,8 @@ static const u32 lt_lcd_regs[] = {
>  void aty_st_lcd(int index, u32 val, const struct atyfb_par *par)
>  {
>  	if (M64_HAS(LT_LCD_REGS)) {
> +		if ((u32)index >= ARRAY_SIZE(lt_lcd_regs))
> +			return;
>  		aty_st_le32(lt_lcd_regs[index], val, par);
>  	} else {
>  		unsigned long temp;
> @@ -164,6 +166,8 @@ void aty_st_lcd(int index, u32 val, const struct atyfb_par *par)
>  u32 aty_ld_lcd(int index, const struct atyfb_par *par)
>  {
>  	if (M64_HAS(LT_LCD_REGS)) {
> +		if ((u32)index >= ARRAY_SIZE(lt_lcd_regs))
> +			return 0;
>  		return aty_ld_le32(lt_lcd_regs[index], par);
>  	} else {
>  		unsigned long temp;

-- 
Jani Nikula, Intel

Re: [PATCH 1/1] fbdev: atyfb: Fix buffer overflow

[Ville Syrjälä]

On Thu, Mar 27, 2025 at 12:14:26PM +0200, Jani Nikula wrote:
> On Thu, 27 Mar 2025, Denis Arefev <arefev@swemel.ru> wrote:
> > The value LCD_MISC_CNTL is used in the 'aty_st_lcd()' function to
> > calculate an index for accessing an array element of size 9.
> > This may cause a buffer overflow.
> 
> The fix is to fix it, not silently brush it under the carpet.

There's actually nothing to fix. The backlight code is only
used on Rage Mobility which has real indexed LCD registers.

Older chips do supposedly have backlight control as well,
but implemented differently. I was mildly curious about
this stuff, so I I poked at my Rage LT Pro a bit to see
if I could get backlight control working on it, but the
only things I was able to achieve were either backlight
completely off, or blinking horribly. So looks like at least
on this machine (Dell Insipiron 7000) the backlight is
implemented in a way that can't be controller via the
normal registers. The machine does have brightness keys that
do work (though the difference between the min and max is
barely noticeable) but they don't result in any changes in
the relevant registers.

> 
> BR,
> Jani.
> 
> >
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Denis Arefev <arefev@swemel.ru>
> > ---
> >  drivers/video/fbdev/aty/atyfb_base.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
> > index 210fd3ac18a4..93eb5eb6042b 100644
> > --- a/drivers/video/fbdev/aty/atyfb_base.c
> > +++ b/drivers/video/fbdev/aty/atyfb_base.c
> > @@ -149,6 +149,8 @@ static const u32 lt_lcd_regs[] = {
> >  void aty_st_lcd(int index, u32 val, const struct atyfb_par *par)
> >  {
> >  	if (M64_HAS(LT_LCD_REGS)) {
> > +		if ((u32)index >= ARRAY_SIZE(lt_lcd_regs))
> > +			return;
> >  		aty_st_le32(lt_lcd_regs[index], val, par);
> >  	} else {
> >  		unsigned long temp;
> > @@ -164,6 +166,8 @@ void aty_st_lcd(int index, u32 val, const struct atyfb_par *par)
> >  u32 aty_ld_lcd(int index, const struct atyfb_par *par)
> >  {
> >  	if (M64_HAS(LT_LCD_REGS)) {
> > +		if ((u32)index >= ARRAY_SIZE(lt_lcd_regs))
> > +			return 0;
> >  		return aty_ld_le32(lt_lcd_regs[index], par);
> >  	} else {
> >  		unsigned long temp;
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/1] fbdev: atyfb: Fix buffer overflow

[Denis Arefev]

Hi Ville. Hi Jani.
Thank you for your answers.

One small question. 
This chip (3D RAGE LT (Mach64 LG)) is very old it is 25 or 
maybe 30 years old, why is it not removed from the core?

Regards Denis.

Re: [PATCH 1/1] fbdev: atyfb: Fix buffer overflow

[Helge Deller]

On 4/1/25 10:40, Denis Arefev wrote:
> One small question.
> This chip (3D RAGE LT (Mach64 LG)) is very old it is 25 or
> maybe 30 years old, why is it not removed from the core?

It's old, but still runs in some configurations and people
still (although probably not on daily bases) use it.
Also don't forget about the various old non-x86 hardware machines
which often used ATI cards too, and those machines are still
supported by Linux as well.

Helge

Re: [PATCH 1/1] fbdev: atyfb: Fix buffer overflow

[Denis Arefev]

> It's old, but still runs in some configurations and people
> still (although probably not on daily bases) use it.
> Also don't forget about the various old non-x86 hardware machines
> which often used ATI cards too, and those machines are still
> supported by Linux as well.

Hi Helge.
Thanks for the reply.

 Ok. Everyone agrees that there is an error (buffer overflow 
lt_lcd_regs[LCD_MISC_CNTL]).
 Ok. Everyone agrees that this code is still needed.

Then I propose to fix this error.  :)

Unfortunately, I can't do everything by the rules, I didn't save
the chip datasheet. (I didn't think I would ever need it again.). 

Regards Denis.

Re: [PATCH 1/1] fbdev: atyfb: Fix buffer overflow

[Ville Syrjälä]

On Tue, Apr 01, 2025 at 01:23:30PM +0300, Denis Arefev wrote:
> > It's old, but still runs in some configurations and people
> > still (although probably not on daily bases) use it.
> > Also don't forget about the various old non-x86 hardware machines
> > which often used ATI cards too, and those machines are still
> > supported by Linux as well.
> 
> Hi Helge.
> Thanks for the reply.
> 
>  Ok. Everyone agrees that there is an error (buffer overflow 
> lt_lcd_regs[LCD_MISC_CNTL]).

As I said, that will never happen.

>  Ok. Everyone agrees that this code is still needed.
> 
> Then I propose to fix this error.  :)
> 
> Unfortunately, I can't do everything by the rules, I didn't save
> the chip datasheet. (I didn't think I would ever need it again.). 
> 
> Regards Denis.

-- 
Ville Syrjälä
Intel