fbdev: off by one test (harmless)

Commit Message

Tomi Valkeinen Jan. 15, 2015, 11:56 a.m. UTC

On 13/01/15 13:19, Tomi Valkeinen wrote:
> On 29/12/14 23:51, David Ung wrote:
>>
>>>> test is really a no-op.  But it's still off by one and upsets the
>>>> static checkers so we may as well correct it.
>>>
>>> 'idx' should be 0~63, because cea_modes array is defined as
>>> 'cea_modes[64]'.
>>
>> According to CEA/EIA-861E, there are 64 defined modes, but idx is valid for 1-64, 0 is reserved hence the check for 
>>
>>         If (!idx || idx > 63) {
>>
>> Think idx check really should be !idx || idx > 64 if following CEA/EIA-861E
> 
> In that case there's something funny with the code. The code indexes
> 'cea_modes' using 'idx', and I _think_ cea_modes is already offset
> properly, i.e. there's no entry at 0. But its length is 64, which is not
> right, as there's the empty item in the beginning.
> 
> So maybe the correct fix is to increase the length of cea_modes to 65,
> and change the idx check as you mention above?

Here's a fix for the cea_array size. Does it silence the static checker?

 Tomi


From ec1edd4683ca3cc0b3b6e5feffae861e404d16d2 Mon Sep 17 00:00:00 2001
From: Tomi Valkeinen <[e-mail]>
Date: Thu, 15 Jan 2015 13:47:19 +0200
Subject: [PATCH] fbdev: fix cea_modes array size

CEA defines 64 modes, indexed from 1 to 64. modedb has cea_modes arrays,
which contains 64 entries. However, the code uses the CEA indices
directly, i.e. the first mode is at cea_modes[1]. This means the array
is one too short.

This does not cause references to uninitialized memory as the code in
fbmon only allows indexes up to 63, and the cea_modes does not contain
an entry for the mode 64 so it could not be used in any case.

However, the code contains a check 'if (idx > ARRAY_SIZE(cea_modes)',
and while that check is a no-op as at that point idx cannot be >= 63, it
upsets static checkers.

Fix this by increasing the cea_array size to be 65, and change the code
to allow mode 64.

Signed-off-by: Tomi Valkeinen <[e-mail]>
---
 drivers/video/fbdev/core/fbmon.c  | 2 +-
 drivers/video/fbdev/core/modedb.c | 2 +-
 include/linux/fb.h                | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Comments

Dan Carpenter Jan. 15, 2015, 12:26 p.m. UTC | #1

On Thu, Jan 15, 2015 at 01:56:58PM +0200, Tomi Valkeinen wrote:
> 
> Here's a fix for the cea_array size. Does it silence the static checker?
> 

No.

>  	for (i = specs->modedb_len + num; i < specs->modedb_len + num + svd_n; i++) {
>  		int idx = svd[i - specs->modedb_len - num];
> -		if (!idx || idx > 63) {
> +		if (!idx || idx > 64) {
>  			pr_warning("Reserved SVD code %d\n", idx);
>  		} else if (idx > ARRAY_SIZE(cea_modes) || !cea_modes[idx].xres) {
>  			pr_warning("Unimplemented SVD code %d\n", idx);

This static check ignores the value of "idx".

It is only looking at "idx > ARRAY_SIZE(cea_modes)" which is off by one
and "idx" is used as an index in "!cea_modes[idx].xres".

The thinking behind this static checker warnings is that off-by-one bugs
are an easy way to boost your patch count even if it's impossible to hit
them in real life.  :)  The value of "idx" is deliberately ignored.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tomi Valkeinen Jan. 15, 2015, 12:39 p.m. UTC | #2

On 15/01/15 14:26, Dan Carpenter wrote:
> On Thu, Jan 15, 2015 at 01:56:58PM +0200, Tomi Valkeinen wrote:
>>
>> Here's a fix for the cea_array size. Does it silence the static checker?
>>
> 
> No.
> 
>>  	for (i = specs->modedb_len + num; i < specs->modedb_len + num + svd_n; i++) {
>>  		int idx = svd[i - specs->modedb_len - num];
>> -		if (!idx || idx > 63) {
>> +		if (!idx || idx > 64) {
>>  			pr_warning("Reserved SVD code %d\n", idx);
>>  		} else if (idx > ARRAY_SIZE(cea_modes) || !cea_modes[idx].xres) {
>>  			pr_warning("Unimplemented SVD code %d\n", idx);
> 
> This static check ignores the value of "idx".
> 
> It is only looking at "idx > ARRAY_SIZE(cea_modes)" which is off by one
> and "idx" is used as an index in "!cea_modes[idx].xres".

Ah, right, I missed that in my patch. The check should obviously be
changed to:

if (idx >= ARRAY_SIZE(cea_modes)

But maybe it would be better to clean up the whole if statement to
remove the duplicate check. So in addition to increasing the cea_array size:

if (idx == 0 || idx >= ARRAY_SIZE(cea_modes)) {
	pr_warning("Reserved SVD code %d\n", idx);
} else if (!cea_modes[idx].xres) {
	pr_warning("Unimplemented SVD code %d\n", idx);

 Tomi

Dan Carpenter Jan. 15, 2015, 12:43 p.m. UTC | #3

Sounds good to me.  :)  Could I get a Reported-by cookie?

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/video/fbdev/core/fbmon.c b/drivers/video/fbdev/core/fbmon.c
index 5b0e313849bd..0fea923c3cb6 100644
--- a/drivers/video/fbdev/core/fbmon.c
+++ b/drivers/video/fbdev/core/fbmon.c
@@ -1059,7 +1059,7 @@  void fb_edid_add_monspecs(unsigned char *edid, struct fb_monspecs *specs)
 
 	for (i = specs->modedb_len + num; i < specs->modedb_len + num + svd_n; i++) {
 		int idx = svd[i - specs->modedb_len - num];
-		if (!idx || idx > 63) {
+		if (!idx || idx > 64) {
 			pr_warning("Reserved SVD code %d\n", idx);
 		} else if (idx > ARRAY_SIZE(cea_modes) || !cea_modes[idx].xres) {
 			pr_warning("Unimplemented SVD code %d\n", idx);
diff --git a/drivers/video/fbdev/core/modedb.c b/drivers/video/fbdev/core/modedb.c
index 388f7971494b..b2aba4e3150a 100644
--- a/drivers/video/fbdev/core/modedb.c
+++ b/drivers/video/fbdev/core/modedb.c
@@ -289,7 +289,7 @@  static const struct fb_videomode modedb[] = {
 };
 
 #ifdef CONFIG_FB_MODE_HELPERS
-const struct fb_videomode cea_modes[64] = {
+const struct fb_videomode cea_modes[65] = {
 	/* #1: 640x480p@59.94/60Hz */
 	[1] = {
 		NULL, 60, 640, 480, 39722, 48, 16, 33, 10, 96, 2, 0,
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 09bb7a18d287..024260687698 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -779,7 +779,7 @@  struct fb_videomode {
 
 extern const char *fb_mode_option;
 extern const struct fb_videomode vesa_modes[];
-extern const struct fb_videomode cea_modes[64];
+extern const struct fb_videomode cea_modes[65];
 
 struct fb_modelist {
 	struct list_head list;