Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1428757 > unrolled thread

[PATCH] fbdev: atyfb: fix array overflow

Started byArnd Bergmann <arnd@arndb.de>
First post2016-06-22 14:40 +0200
Last post2016-06-23 11:30 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] fbdev: atyfb: fix array overflow Arnd Bergmann <arnd@arndb.de> - 2016-06-22 14:40 +0200
    Re: [PATCH] fbdev: atyfb: fix array overflow Ville Syrjälä <syrjala@sci.fi> - 2016-06-23 02:40 +0200
      Re: [PATCH] fbdev: atyfb: fix array overflow Arnd Bergmann <arnd@arndb.de> - 2016-06-23 11:10 +0200
    Re: [PATCH] fbdev: atyfb: fix array overflow Geert Uytterhoeven <geert@linux-m68k.org> - 2016-06-23 11:00 +0200
      Re: [PATCH] fbdev: atyfb: fix array overflow Arnd Bergmann <arnd@arndb.de> - 2016-06-23 11:30 +0200

#1428757 — [PATCH] fbdev: atyfb: fix array overflow

FromArnd Bergmann <arnd@arndb.de>
Date2016-06-22 14:40 +0200
Subject[PATCH] fbdev: atyfb: fix array overflow
Message-ID<rMPBM-3lU-31@gated-at.bofh.it>
When building with CONFIG_UBSAN_SANITIZE_ALL on ARM, I get this
gcc warning for atyfb:

drivers/video/fbdev/aty/atyfb_base.c: In function 'aty_bl_update_status':
drivers/video/fbdev/aty/atyfb_base.c:167:33: warning: array subscript is above array bounds [-Warray-bounds]
drivers/video/fbdev/aty/atyfb_base.c:152:26: warning: array subscript is above array bounds [-Warray-bounds]

Apparently the warning is correct and there is indeed an overflow,
which was never caught. I could only reproduce this on ARM and
have opened a bug against the compiler for the lack of warning.

This patch makes the array larger, so we cover all possible
registers in the LCD controller without an overflow.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Link: https://bugs.linaro.org/show_bug.cgi?id=2349
---
 drivers/video/fbdev/aty/atyfb_base.c | 2 +-
 include/video/mach64.h               | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
index 001d3d871800..36ffba152eab 100644
--- a/drivers/video/fbdev/aty/atyfb_base.c
+++ b/drivers/video/fbdev/aty/atyfb_base.c
@@ -134,7 +134,7 @@
 
 #if defined(CONFIG_PM) || defined(CONFIG_PMAC_BACKLIGHT) || \
 defined (CONFIG_FB_ATY_GENERIC_LCD) || defined(CONFIG_FB_ATY_BACKLIGHT)
-static const u32 lt_lcd_regs[] = {
+static const u32 lt_lcd_regs[LCD_REG_NUM] = {
 	CNFG_PANEL_LG,
 	LCD_GEN_CNTL_LG,
 	DSTN_CONTROL_LG,
diff --git a/include/video/mach64.h b/include/video/mach64.h
index 89e91c0cb737..9f74e9e0aeb8 100644
--- a/include/video/mach64.h
+++ b/include/video/mach64.h
@@ -1299,6 +1299,7 @@
 #define APC_LUT_KL		0x38
 #define APC_LUT_MN		0x39
 #define APC_LUT_OP		0x3A
+#define LCD_REG_NUM		0x3B /* total number */
 
 /* Values in LCD_GEN_CTRL */
 #define CRT_ON                          0x00000001ul
-- 
2.9.0

[toc] | [next] | [standalone]

#1429363

FromVille Syrjälä <syrjala@sci.fi>
Date2016-06-23 02:40 +0200
Message-ID<rN0Qx-2iS-7@gated-at.bofh.it>
In reply to#1428757
On Wed, Jun 22, 2016 at 02:37:11PM +0200, Arnd Bergmann wrote:
> When building with CONFIG_UBSAN_SANITIZE_ALL on ARM, I get this
> gcc warning for atyfb:
> 
> drivers/video/fbdev/aty/atyfb_base.c: In function 'aty_bl_update_status':
> drivers/video/fbdev/aty/atyfb_base.c:167:33: warning: array subscript is above array bounds [-Warray-bounds]
> drivers/video/fbdev/aty/atyfb_base.c:152:26: warning: array subscript is above array bounds [-Warray-bounds]
> 
> Apparently the warning is correct and there is indeed an overflow,

Nope. All the LCD register indexes on the Rage LT (the only relevant
chip for this code path) should stay below the table size. At least
I can't see any place where we'd walk past the end.

> which was never caught. I could only reproduce this on ARM and
> have opened a bug against the compiler for the lack of warning.
> 
> This patch makes the array larger, so we cover all possible
> registers in the LCD controller without an overflow.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Link: https://bugs.linaro.org/show_bug.cgi?id=2349
> ---
>  drivers/video/fbdev/aty/atyfb_base.c | 2 +-
>  include/video/mach64.h               | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
> index 001d3d871800..36ffba152eab 100644
> --- a/drivers/video/fbdev/aty/atyfb_base.c
> +++ b/drivers/video/fbdev/aty/atyfb_base.c
> @@ -134,7 +134,7 @@
>  
>  #if defined(CONFIG_PM) || defined(CONFIG_PMAC_BACKLIGHT) || \
>  defined (CONFIG_FB_ATY_GENERIC_LCD) || defined(CONFIG_FB_ATY_BACKLIGHT)
> -static const u32 lt_lcd_regs[] = {
> +static const u32 lt_lcd_regs[LCD_REG_NUM] = {
>  	CNFG_PANEL_LG,
>  	LCD_GEN_CNTL_LG,
>  	DSTN_CONTROL_LG,
> diff --git a/include/video/mach64.h b/include/video/mach64.h
> index 89e91c0cb737..9f74e9e0aeb8 100644
> --- a/include/video/mach64.h
> +++ b/include/video/mach64.h
> @@ -1299,6 +1299,7 @@
>  #define APC_LUT_KL		0x38
>  #define APC_LUT_MN		0x39
>  #define APC_LUT_OP		0x3A
> +#define LCD_REG_NUM		0x3B /* total number */
>  
>  /* Values in LCD_GEN_CTRL */
>  #define CRT_ON                          0x00000001ul
> -- 
> 2.9.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

[toc] | [prev] | [next] | [standalone]

#1429618

FromArnd Bergmann <arnd@arndb.de>
Date2016-06-23 11:10 +0200
Message-ID<rN8O6-7LB-11@gated-at.bofh.it>
In reply to#1429363
On Thursday, June 23, 2016 3:28:25 AM CEST Ville Syrjälä wrote:
> On Wed, Jun 22, 2016 at 02:37:11PM +0200, Arnd Bergmann wrote:
> > When building with CONFIG_UBSAN_SANITIZE_ALL on ARM, I get this
> > gcc warning for atyfb:
> > 
> > drivers/video/fbdev/aty/atyfb_base.c: In function 'aty_bl_update_status':
> > drivers/video/fbdev/aty/atyfb_base.c:167:33: warning: array subscript is above array bounds [-Warray-bounds]
> > drivers/video/fbdev/aty/atyfb_base.c:152:26: warning: array subscript is above array bounds [-Warray-bounds]
> > 
> > Apparently the warning is correct and there is indeed an overflow,
> 
> Nope. All the LCD register indexes on the Rage LT (the only relevant
> chip for this code path) should stay below the table size. At least
> I can't see any place where we'd walk past the end.

I don't understand what you mean: the warning is about LCD_MISC_CNTL,
which is defined as 0x14, while the array size is 9 and that is smaller.

Is there something more subtle going on than what gcc sees?

	Arnd

[toc] | [prev] | [next] | [standalone]

#1429604

FromGeert Uytterhoeven <geert@linux-m68k.org>
Date2016-06-23 11:00 +0200
Message-ID<rN8Eq-7sH-19@gated-at.bofh.it>
In reply to#1428757
Hi Arnd,

On Wed, Jun 22, 2016 at 2:37 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> When building with CONFIG_UBSAN_SANITIZE_ALL on ARM, I get this
> gcc warning for atyfb:
>
> drivers/video/fbdev/aty/atyfb_base.c: In function 'aty_bl_update_status':
> drivers/video/fbdev/aty/atyfb_base.c:167:33: warning: array subscript is above array bounds [-Warray-bounds]
> drivers/video/fbdev/aty/atyfb_base.c:152:26: warning: array subscript is above array bounds [-Warray-bounds]
>
> Apparently the warning is correct and there is indeed an overflow,
> which was never caught. I could only reproduce this on ARM and
> have opened a bug against the compiler for the lack of warning.
>
> This patch makes the array larger, so we cover all possible
> registers in the LCD controller without an overflow.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Link: https://bugs.linaro.org/show_bug.cgi?id=2349
> ---
>  drivers/video/fbdev/aty/atyfb_base.c | 2 +-
>  include/video/mach64.h               | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
> index 001d3d871800..36ffba152eab 100644
> --- a/drivers/video/fbdev/aty/atyfb_base.c
> +++ b/drivers/video/fbdev/aty/atyfb_base.c
> @@ -134,7 +134,7 @@
>
>  #if defined(CONFIG_PM) || defined(CONFIG_PMAC_BACKLIGHT) || \
>  defined (CONFIG_FB_ATY_GENERIC_LCD) || defined(CONFIG_FB_ATY_BACKLIGHT)
> -static const u32 lt_lcd_regs[] = {
> +static const u32 lt_lcd_regs[LCD_REG_NUM] = {
>         CNFG_PANEL_LG,
>         LCD_GEN_CNTL_LG,
>         DSTN_CONTROL_LG,
> diff --git a/include/video/mach64.h b/include/video/mach64.h
> index 89e91c0cb737..9f74e9e0aeb8 100644
> --- a/include/video/mach64.h
> +++ b/include/video/mach64.h
> @@ -1299,6 +1299,7 @@
>  #define APC_LUT_KL             0x38
>  #define APC_LUT_MN             0x39
>  #define APC_LUT_OP             0x3A
> +#define LCD_REG_NUM            0x3B /* total number */
>
>  /* Values in LCD_GEN_CTRL */
>  #define CRT_ON                          0x00000001ul

This doesn't look like the right fix to me.

Before, aty_st_lcd(LCD_MISC_CNTL, reg, par) in aty_bl_update_status()
wrote into an arbitrary register.
With your fix, it will write to register 0, which is IMHO also not OK.

I think aty_st_lcd() and aty_ld_lcd() should check whether the index is
out of range, perhaps even with a WARN_ON()?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1429636

FromArnd Bergmann <arnd@arndb.de>
Date2016-06-23 11:30 +0200
Message-ID<rN97r-7UZ-19@gated-at.bofh.it>
In reply to#1429604
On Thursday, June 23, 2016 10:50:04 AM CEST Geert Uytterhoeven wrote:
> Hi Arnd,
> 
> On Wed, Jun 22, 2016 at 2:37 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > When building with CONFIG_UBSAN_SANITIZE_ALL on ARM, I get this
> > gcc warning for atyfb:
> >
> > drivers/video/fbdev/aty/atyfb_base.c: In function 'aty_bl_update_status':
> > drivers/video/fbdev/aty/atyfb_base.c:167:33: warning: array subscript is above array bounds [-Warray-bounds]
> > drivers/video/fbdev/aty/atyfb_base.c:152:26: warning: array subscript is above array bounds [-Warray-bounds]
> >
> > Apparently the warning is correct and there is indeed an overflow,
> > which was never caught. I could only reproduce this on ARM and
> > have opened a bug against the compiler for the lack of warning.
> >
> > This patch makes the array larger, so we cover all possible
> > registers in the LCD controller without an overflow.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > Link: https://bugs.linaro.org/show_bug.cgi?id=2349
> > ---
> >  drivers/video/fbdev/aty/atyfb_base.c | 2 +-
> >  include/video/mach64.h               | 1 +
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
> > index 001d3d871800..36ffba152eab 100644
> > --- a/drivers/video/fbdev/aty/atyfb_base.c
> > +++ b/drivers/video/fbdev/aty/atyfb_base.c
> > @@ -134,7 +134,7 @@
> >
> >  #if defined(CONFIG_PM) || defined(CONFIG_PMAC_BACKLIGHT) || \
> >  defined (CONFIG_FB_ATY_GENERIC_LCD) || defined(CONFIG_FB_ATY_BACKLIGHT)
> > -static const u32 lt_lcd_regs[] = {
> > +static const u32 lt_lcd_regs[LCD_REG_NUM] = {
> >         CNFG_PANEL_LG,
> >         LCD_GEN_CNTL_LG,
> >         DSTN_CONTROL_LG,
> > diff --git a/include/video/mach64.h b/include/video/mach64.h
> > index 89e91c0cb737..9f74e9e0aeb8 100644
> > --- a/include/video/mach64.h
> > +++ b/include/video/mach64.h
> > @@ -1299,6 +1299,7 @@
> >  #define APC_LUT_KL             0x38
> >  #define APC_LUT_MN             0x39
> >  #define APC_LUT_OP             0x3A
> > +#define LCD_REG_NUM            0x3B /* total number */
> >
> >  /* Values in LCD_GEN_CTRL */
> >  #define CRT_ON                          0x00000001ul
> 
> This doesn't look like the right fix to me.
> 
> Before, aty_st_lcd(LCD_MISC_CNTL, reg, par) in aty_bl_update_status()
> wrote into an arbitrary register.
> With your fix, it will write to register 0, which is IMHO also not OK.

Ok, I see now. I thought it array was for initializing the registers and
caching the contents as some other drivers do it, but it's really used
for some indirect addressing method.

> I think aty_st_lcd() and aty_ld_lcd() should check whether the index is
> out of range, perhaps even with a WARN_ON()?

Just guessing what the right behavior would be, maybe something like
this? That would assume that the LCD_MISC_CNTL is accessible
through LCD_INDEX/LCD_DATA but not through a direct register.

	Arnd

diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
index 36ffba152eab..c67d4b767e9a 100644
--- a/drivers/video/fbdev/aty/atyfb_base.c
+++ b/drivers/video/fbdev/aty/atyfb_base.c
@@ -148,7 +148,7 @@ static const u32 lt_lcd_regs[LCD_REG_NUM] = {
 
 void aty_st_lcd(int index, u32 val, const struct atyfb_par *par)
 {
-	if (M64_HAS(LT_LCD_REGS)) {
+	if (M64_HAS(LT_LCD_REGS) && index < ARRAY_SIZE(lt_lcd_regs)) {
 		aty_st_le32(lt_lcd_regs[index], val, par);
 	} else {
 		unsigned long temp;
@@ -163,7 +163,7 @@ 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 (M64_HAS(LT_LCD_REGS) && index < ARRAY_SIZE(lt_lcd_regs)) {
 		return aty_ld_le32(lt_lcd_regs[index], par);
 	} else {
 		unsigned long temp;

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web