All of lore.kernel.org
 help / color / mirror / Atom feed
* [patch] fbdev: off by one test (harmless)
@ 2014-12-26 17:26 Dan Carpenter
  2014-12-27  1:29 ` Jingoo Han
                   ` (11 more replies)
  0 siblings, 12 replies; 13+ messages in thread
From: Dan Carpenter @ 2014-12-26 17:26 UTC (permalink / raw)
  To: linux-fbdev

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 <dan.carpenter@oracle.com>

diff --git a/drivers/video/fbdev/core/fbmon.c b/drivers/video/fbdev/core/fbmon.c
index 5b0e313..3030269 100644
--- a/drivers/video/fbdev/core/fbmon.c
+++ b/drivers/video/fbdev/core/fbmon.c
@@ -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]));

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [patch] fbdev: off by one test (harmless)
  2014-12-26 17:26 [patch] fbdev: off by one test (harmless) Dan Carpenter
@ 2014-12-27  1:29 ` Jingoo Han
  2014-12-29 21:51 ` David Ung
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Jingoo Han @ 2014-12-27  1:29 UTC (permalink / raw)
  To: linux-fbdev

On Saturday, December 27, 2014 2:27 AM, Dan Carpenter wrote:
> 
> 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];

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

> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Reviewed-by: Jingoo Han <jg1.han@samsung.com>

Best regards,
Jingoo Han

> 
> diff --git a/drivers/video/fbdev/core/fbmon.c b/drivers/video/fbdev/core/fbmon.c
> index 5b0e313..3030269 100644
> --- a/drivers/video/fbdev/core/fbmon.c
> +++ b/drivers/video/fbdev/core/fbmon.c
> @@ -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]));


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [patch] fbdev: off by one test (harmless)
  2014-12-26 17:26 [patch] fbdev: off by one test (harmless) Dan Carpenter
  2014-12-27  1:29 ` Jingoo Han
@ 2014-12-29 21:51 ` David Ung
  2015-01-13 11:19 ` Tomi Valkeinen
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: David Ung @ 2014-12-29 21:51 UTC (permalink / raw)
  To: linux-fbdev


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [patch] fbdev: off by one test (harmless)
  2014-12-26 17:26 [patch] fbdev: off by one test (harmless) Dan Carpenter
  2014-12-27  1:29 ` Jingoo Han
  2014-12-29 21:51 ` David Ung
@ 2015-01-13 11:19 ` Tomi Valkeinen
  2015-01-14  5:24 ` David Ung
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Tomi Valkeinen @ 2015-01-13 11:19 UTC (permalink / raw)
  To: linux-fbdev

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

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?

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [patch] fbdev: off by one test (harmless)
  2014-12-26 17:26 [patch] fbdev: off by one test (harmless) Dan Carpenter
                   ` (2 preceding siblings ...)
  2015-01-13 11:19 ` Tomi Valkeinen
@ 2015-01-14  5:24 ` David Ung
  2015-01-15 11:42 ` Tomi Valkeinen
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: David Ung @ 2015-01-14  5:24 UTC (permalink / raw)
  To: linux-fbdev


> -----Original Message-----
> From: Tomi Valkeinen [mailto:tomi.valkeinen@ti.com]
> 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'; linux-fbdev@vger.kernel.org; kernel-janitors@vger.kernel.org
> Subject: Re: [patch] fbdev: off by one test (harmless)
> 
> * PGP Signed by an unknown key
> 
> 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?
> 

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [patch] fbdev: off by one test (harmless)
  2014-12-26 17:26 [patch] fbdev: off by one test (harmless) Dan Carpenter
                   ` (3 preceding siblings ...)
  2015-01-14  5:24 ` David Ung
@ 2015-01-15 11:42 ` Tomi Valkeinen
  2015-01-15 11:56 ` Tomi Valkeinen
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Tomi Valkeinen @ 2015-01-15 11:42 UTC (permalink / raw)
  To: linux-fbdev

[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]

On 14/01/15 07:24, David Ung wrote:
> 
>> -----Original Message-----
>> From: Tomi Valkeinen [mailto:tomi.valkeinen@ti.com]
>> 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'; linux-fbdev@vger.kernel.org; kernel-janitors@vger.kernel.org
>> Subject: Re: [patch] fbdev: off by one test (harmless)
>>
>> * PGP Signed by an unknown key
>>
>> 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?
>>
> 
> 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [patch] fbdev: off by one test (harmless)
  2014-12-26 17:26 [patch] fbdev: off by one test (harmless) Dan Carpenter
                   ` (4 preceding siblings ...)
  2015-01-15 11:42 ` Tomi Valkeinen
@ 2015-01-15 11:56 ` Tomi Valkeinen
  2015-01-15 12:26 ` Dan Carpenter
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Tomi Valkeinen @ 2015-01-15 11:56 UTC (permalink / raw)
  To: linux-fbdev

[-- Attachment #1: Type: text/plain, Size: 3817 bytes --]

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 <tomi.valkeinen@ti.com>
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 <tomi.valkeinen@ti.com>
---
 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(-)

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;
-- 
2.2.2




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [patch] fbdev: off by one test (harmless)
  2014-12-26 17:26 [patch] fbdev: off by one test (harmless) Dan Carpenter
                   ` (5 preceding siblings ...)
  2015-01-15 11:56 ` Tomi Valkeinen
@ 2015-01-15 12:26 ` Dan Carpenter
  2015-01-15 12:39 ` Tomi Valkeinen
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2015-01-15 12:26 UTC (permalink / raw)
  To: linux-fbdev

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [patch] fbdev: off by one test (harmless)
  2014-12-26 17:26 [patch] fbdev: off by one test (harmless) Dan Carpenter
                   ` (6 preceding siblings ...)
  2015-01-15 12:26 ` Dan Carpenter
@ 2015-01-15 12:39 ` Tomi Valkeinen
  2015-01-15 12:43 ` Dan Carpenter
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Tomi Valkeinen @ 2015-01-15 12:39 UTC (permalink / raw)
  To: linux-fbdev

[-- Attachment #1: Type: text/plain, Size: 1240 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [patch] fbdev: off by one test (harmless)
  2014-12-26 17:26 [patch] fbdev: off by one test (harmless) Dan Carpenter
                   ` (7 preceding siblings ...)
  2015-01-15 12:39 ` Tomi Valkeinen
@ 2015-01-15 12:43 ` Dan Carpenter
  2015-01-15 12:52 ` Tomi Valkeinen
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2015-01-15 12:43 UTC (permalink / raw)
  To: linux-fbdev

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

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [patch] fbdev: off by one test (harmless)
  2014-12-26 17:26 [patch] fbdev: off by one test (harmless) Dan Carpenter
                   ` (8 preceding siblings ...)
  2015-01-15 12:43 ` Dan Carpenter
@ 2015-01-15 12:52 ` Tomi Valkeinen
  2015-07-23 12:39 ` Dan Carpenter
  2015-08-10  9:38 ` Tomi Valkeinen
  11 siblings, 0 replies; 13+ messages in thread
From: Tomi Valkeinen @ 2015-01-15 12:52 UTC (permalink / raw)
  To: linux-fbdev

[-- Attachment #1: Type: text/plain, Size: 3232 bytes --]

On 15/01/15 14:43, Dan Carpenter wrote:
> 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 <tomi.valkeinen@ti.com>
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 <tomi.valkeinen@ti.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 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(-)

diff --git a/drivers/video/fbdev/core/fbmon.c b/drivers/video/fbdev/core/fbmon.c
index 5b0e313849bd..1829fe34a1df 100644
--- a/drivers/video/fbdev/core/fbmon.c
+++ b/drivers/video/fbdev/core/fbmon.c
@@ -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]));
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;
-- 
2.2.2




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [patch] fbdev: off by one test (harmless)
  2014-12-26 17:26 [patch] fbdev: off by one test (harmless) Dan Carpenter
                   ` (9 preceding siblings ...)
  2015-01-15 12:52 ` Tomi Valkeinen
@ 2015-07-23 12:39 ` Dan Carpenter
  2015-08-10  9:38 ` Tomi Valkeinen
  11 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2015-07-23 12:39 UTC (permalink / raw)
  To: linux-fbdev

This was never merged.

regards,
dan carpenter

On Thu, Jan 15, 2015 at 02:52:29PM +0200, Tomi Valkeinen wrote:
> On 15/01/15 14:43, Dan Carpenter wrote:
> > 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 <tomi.valkeinen@ti.com>
> 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 <tomi.valkeinen@ti.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  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(-)
> 
> diff --git a/drivers/video/fbdev/core/fbmon.c b/drivers/video/fbdev/core/fbmon.c
> index 5b0e313849bd..1829fe34a1df 100644
> --- a/drivers/video/fbdev/core/fbmon.c
> +++ b/drivers/video/fbdev/core/fbmon.c
> @@ -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]));
> 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;
> -- 
> 2.2.2
> 
> 
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [patch] fbdev: off by one test (harmless)
  2014-12-26 17:26 [patch] fbdev: off by one test (harmless) Dan Carpenter
                   ` (10 preceding siblings ...)
  2015-07-23 12:39 ` Dan Carpenter
@ 2015-08-10  9:38 ` Tomi Valkeinen
  11 siblings, 0 replies; 13+ messages in thread
From: Tomi Valkeinen @ 2015-08-10  9:38 UTC (permalink / raw)
  To: linux-fbdev

[-- Attachment #1: Type: text/plain, Size: 112 bytes --]


On 23/07/15 15:39, Dan Carpenter wrote:
> This was never merged.

Oops. I'll pick this up now.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2015-08-10  9:38 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-26 17:26 [patch] fbdev: off by one test (harmless) Dan Carpenter
2014-12-27  1:29 ` Jingoo Han
2014-12-29 21:51 ` David Ung
2015-01-13 11:19 ` Tomi Valkeinen
2015-01-14  5:24 ` David Ung
2015-01-15 11:42 ` Tomi Valkeinen
2015-01-15 11:56 ` Tomi Valkeinen
2015-01-15 12:26 ` Dan Carpenter
2015-01-15 12:39 ` Tomi Valkeinen
2015-01-15 12:43 ` Dan Carpenter
2015-01-15 12:52 ` Tomi Valkeinen
2015-07-23 12:39 ` Dan Carpenter
2015-08-10  9:38 ` Tomi Valkeinen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.