Skip to contentSkip to conversation

fbdev: off by one test (harmless)

PATCH In review Dan Carpenter 4 participants 13 messages Dec 26, 2014 – Aug 10, 2015

Subsystems (3)
Kernel janitors Framebuffer layer Framebuffer core
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 fbdev: off by one test (harmless)

ARRAY_SIZE(cea_modes) is 64 so the "idx > 63" test earlier means this 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.

Signed-off-by Dan Carpenter <[email protected]>
drivers/video/fbdev/core/fbmon.c +1 -1
index 5b0e313..3030269 100644
@@ -1061,7 +1061,7 @@ void fb_edid_add_monspecs(unsigned char *edid, struct fb_monspecs *specs)
 		int idx = svd[i - specs->modedb_len - num];
 		if (!idx || idx > 63) {
 			pr_warning("Reserved SVD code %d\n", idx);
-		} else if (idx > ARRAY_SIZE(cea_modes) || !cea_modes[idx].xres) {
+		} else if (idx >= ARRAY_SIZE(cea_modes) || !cea_modes[idx].xres) {
 			pr_warning("Unimplemented SVD code %d\n", idx);
 		} else {
 			memcpy(&m[i], cea_modes + idx, sizeof(m[i]));
Reviewed-by on v1 1/1
#1 lore raw
Quoted 2 lines · Dan Carpenter

ARRAY_SIZE(cea_modes) is 64 so the "idx > 63" test earlier means this

Good.
'cea_modes' is defined as below. So, ARRAY_SIZE(cea_modes) is 64. extern const struct fb_videomode cea_modes[64];

Quoted 2 lines

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]'.

Quoted 2 lines
Signed-off-by Dan Carpenter <[email protected]>
Reviewed-by Jingoo Han <[email protected]>

Best regards,
Jingoo Han

Quoted 14 lines
drivers/video/fbdev/core/fbmon.c +1 -1
index 5b0e313..3030269 100644
@@ -1061,7 +1061,7 @@ void fb_edid_add_monspecs(unsigned char *edid, struct fb_monspecs *specs)
 		int idx = svd[i - specs->modedb_len - num];
 		if (!idx || idx > 63) {
 			pr_warning("Reserved SVD code %d\n", idx);
-		} else if (idx > ARRAY_SIZE(cea_modes) || !cea_modes[idx].xres) {
+		} else if (idx >= ARRAY_SIZE(cea_modes) || !cea_modes[idx].xres) {
 			pr_warning("Unimplemented SVD code %d\n", idx);
 		} else {
 			memcpy(&m[i], cea_modes + idx, sizeof(m[i]));
#2 lore raw
In reply to Dan Carpenter
Quoted 6 lines
Quoted 3 lines

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

CEA/EIA-861F add additional entries and idx is valid from 1-107

David
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

#3 lore raw
In reply to Dan Carpenter
Quoted 4 lines · David Ung
Quoted 4 lines
Quoted 3 lines

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.

Quoted 8 lines
Quoted 2 lines

'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?

Tomi

#4 lore raw
In reply to Dan Carpenter
Quoted 27 lines

-----Original Message-----
From: Tomi Valkeinen [mailto:[email protected]]
Sent: Tuesday, January 13, 2015 3:19 AM
To: David Ung; 'Jingoo Han'; 'Dan Carpenter'
Cc: 'Jean-Christophe Plagniol-Villard'; 'Daniel Vetter'; 'Laurent Pinchart'; 'Rob Clark'; [email protected]; [email protected]
Subject: Re: [patch] fbdev: off by one test (harmless)

* PGP Signed by an unknown key

Quoted 7 lines · David Ung
Quoted 6 lines
Quoted 3 lines

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]'.

Quoted 7 lines

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

Quoted 8 lines

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?

In that case might aswell go with CEA/EAI-861F for completeness and have the check against 107. but with a slight waste of memory.

David
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

#5 lore raw
In reply to Dan Carpenter
Quoted 32 lines · David Ung
Quoted 32 lines

-----Original Message-----
From: Tomi Valkeinen [mailto:[email protected]]
Sent: Tuesday, January 13, 2015 3:19 AM
To: David Ung; 'Jingoo Han'; 'Dan Carpenter'
Cc: 'Jean-Christophe Plagniol-Villard'; 'Daniel Vetter'; 'Laurent Pinchart'; 'Rob Clark'; [email protected]; [email protected]
Subject: Re: [patch] fbdev: off by one test (harmless)

* PGP Signed by an unknown key

Quoted 7 lines · David Ung
Quoted 6 lines
Quoted 3 lines

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]'.

Quoted 7 lines

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.

Quoted 6 lines
Quoted 3 lines

So maybe the correct fix is to increase the length of cea_modes to 65, and change the idx check as you mention above?

In that case might aswell go with CEA/EAI-861F for completeness and have the check against 107. but with a slight waste of memory.

Then we need to fill in the new modes. Or leave them blank, but then, what would be the point.

And I'd rather not add new stuff to fbdev that is not absolutely needed. Everybody should move to DRM =).

Tomi

#6 lore raw
In reply to Dan Carpenter
Quoted 14 lines · Tomi Valkeinen
Quoted 4 lines · David Ung
Quoted 4 lines
Quoted 3 lines

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.

Quoted 8 lines
Quoted 2 lines

'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

Quoted 7 lines

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 <[email protected]>
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 <[email protected]>
---
 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(-)
drivers/video/fbdev/core/fbmon.c +1 -1
index 5b0e313849bd..0fea923c3cb6 100644
@@ -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);
drivers/video/fbdev/core/modedb.c +1 -1
index 388f7971494b..b2aba4e3150a 100644
@@ -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: [email protected]/60Hz */
 	[1] = {
 		NULL, 60, 640, 480, 39722, 48, 16, 33, 10, 96, 2, 0,
include/linux/fb.h +1 -1
index 09bb7a18d287..024260687698 100644
@@ -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;
Signature · 1 line
2.2.2
author
#7 lore raw
In reply to Dan Carpenter
Quoted 3 lines · Tomi Valkeinen

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

No.

Review

Quoted 7 lines
(quoted diff) +1 -1
 	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

#8 lore raw
In reply to Dan Carpenter
Quoted 17 lines · Dan Carpenter
Quoted 3 lines · Tomi Valkeinen

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

No.

Review

Quoted 7 lines
(quoted diff) +1 -1
 	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".

Quoted 2 lines

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

#10 lore raw
In reply to Dan Carpenter
Quoted 1 line · Dan Carpenter

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

Oh, sorry. I thought I had added that, but... seems I didn't. Well now it's there. So updated patch below.

I'll queue it for 3.20 if there are no comments.

Tomi

From 80fa3aed6f89728e9003bb0bdbd0b4bac2bc2186 Mon Sep 17 00:00:00 2001
From: Tomi Valkeinen <[email protected]>
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 <[email protected]>
Reported-by Dan Carpenter <[email protected]>
---
 drivers/video/fbdev/core/fbmon.c  | 4 ++--
 drivers/video/fbdev/core/modedb.c | 2 +-
 include/linux/fb.h                | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)
drivers/video/fbdev/core/fbmon.c +2 -2
index 5b0e313849bd..1829fe34a1df 100644
@@ -1059,9 +1059,9 @@ 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 >= ARRAY_SIZE(cea_modes)) {
 			pr_warning("Reserved SVD code %d\n", idx);
-		} else if (idx > ARRAY_SIZE(cea_modes) || !cea_modes[idx].xres) {
+		} else if (!cea_modes[idx].xres) {
 			pr_warning("Unimplemented SVD code %d\n", idx);
 		} else {
 			memcpy(&m[i], cea_modes + idx, sizeof(m[i]));
drivers/video/fbdev/core/modedb.c +1 -1
index 388f7971494b..b2aba4e3150a 100644
@@ -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: [email protected]/60Hz */
 	[1] = {
 		NULL, 60, 640, 480, 39722, 48, 16, 33, 10, 96, 2, 0,
include/linux/fb.h +1 -1
index 09bb7a18d287..024260687698 100644
@@ -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;
Signature · 1 line
2.2.2
author
#11 lore raw
In reply to Dan Carpenter

This was never merged.

regards,
dan carpenter

Quoted 87 lines · Tomi Valkeinen
Quoted 1 line · Dan Carpenter

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

Oh, sorry. I thought I had added that, but... seems I didn't. Well now it's there. So updated patch below.

I'll queue it for 3.20 if there are no comments.

Tomi

From 80fa3aed6f89728e9003bb0bdbd0b4bac2bc2186 Mon Sep 17 00:00:00 2001
From: Tomi Valkeinen <[email protected]>
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 <[email protected]>
Reported-by Dan Carpenter <[email protected]>
---
 drivers/video/fbdev/core/fbmon.c  | 4 ++--
 drivers/video/fbdev/core/modedb.c | 2 +-
 include/linux/fb.h                | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)
drivers/video/fbdev/core/fbmon.c +2 -2
index 5b0e313849bd..1829fe34a1df 100644
@@ -1059,9 +1059,9 @@ 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 >= ARRAY_SIZE(cea_modes)) {
 			pr_warning("Reserved SVD code %d\n", idx);
-		} else if (idx > ARRAY_SIZE(cea_modes) || !cea_modes[idx].xres) {
+		} else if (!cea_modes[idx].xres) {
 			pr_warning("Unimplemented SVD code %d\n", idx);
 		} else {
 			memcpy(&m[i], cea_modes + idx, sizeof(m[i]));
drivers/video/fbdev/core/modedb.c +1 -1
index 388f7971494b..b2aba4e3150a 100644
@@ -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: [email protected]/60Hz */
 	[1] = {
 		NULL, 60, 640, 480, 39722, 48, 16, 33, 10, 96, 2, 0,
include/linux/fb.h +1 -1
index 09bb7a18d287..024260687698 100644
@@ -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;
Signature · 1 line
2.2.2