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#49
---
drivers/video/fbdev/aty/atyfb_base.c | 2 +-
include/video/mach64.h | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
From: Ville Syrjälä <syrjala@sci.fi> Date: 2016-06-23 00:35:09
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.
quoted hunk
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#49
---
drivers/video/fbdev/aty/atyfb_base.c | 2 +-
include/video/mach64.h | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
@@ -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
Hi Arnd,
On Wed, Jun 22, 2016 at 2:37 PM, Arnd Bergmann [off-list ref] wrote:
quoted hunk
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#49
---
drivers/video/fbdev/aty/atyfb_base.c | 2 +-
include/video/mach64.h | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
@@ -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
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:
quoted
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
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 [off-list ref] wrote:
quoted
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#49
---
drivers/video/fbdev/aty/atyfb_base.c | 2 +-
include/video/mach64.h | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
@@ -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
From: Ville Syrjälä <syrjala@sci.fi> Date: 2016-06-23 17:26:56
On Thu, Jun 23, 2016 at 11:06:11AM +0200, Arnd Bergmann wrote:
On Thursday, June 23, 2016 3:28:25 AM CEST Ville Syrjälä wrote:
quoted
On Wed, Jun 22, 2016 at 02:37:11PM +0200, Arnd Bergmann wrote:
quoted
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?
The LCD_MISC_CNTL access is in the backlight code, and thanks to the
following piece of code
if (M64_HAS(MOBIL_BUS) && ...) {
aty_bl_init(...);
}
we register the backlight only on Rage Mobility. Rage LT is not a Rage
Mobility, so everything is fine.
--
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
From: Ville Syrjälä <syrjala@sci.fi> Date: 2016-06-23 17:35:22
On Thu, Jun 23, 2016 at 11:22:20AM +0200, Arnd Bergmann wrote:
quoted hunk
On Thursday, June 23, 2016 10:50:04 AM CEST Geert Uytterhoeven wrote:
quoted
Hi Arnd,
On Wed, Jun 22, 2016 at 2:37 PM, Arnd Bergmann [off-list ref] wrote:
quoted
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#49
---
drivers/video/fbdev/aty/atyfb_base.c | 2 +-
include/video/mach64.h | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
@@ -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.
quoted
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
We don't want to take the 'else' branch ever, so this isn't any safer
than the original code. So maybe something like this:
if (M64_HAS(LT_LCD_REGS)) {
if (WARN_ON(index >= ARRAY_SIZE(lt_lcd_regs)))
return;
...
} else {
...
}
But aty_ld_lcd() still needs to return something even if the index
is bogus. Either all ones or all zeroes I guess. Which one is
better? I don't know. Not sure what the hardware gives you when trying
to read an invalid register.
quoted hunk
@@ -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;--
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