From: Dan Carpenter <hidden> Date: 2014-12-26 17:26:57
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 <redacted>
From: David Ung <hidden> Date: 2014-12-29 21:51:23
quoted
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.
-----------------------------------------------------------------------------------
From: Tomi Valkeinen <hidden> Date: 2015-01-13 11:19:17
On 29/12/14 23:51, David Ung wrote:
quoted
quoted
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
From: David Ung <hidden> Date: 2015-01-14 05:24:09
-----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:
quoted
quoted
quoted
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.
-----------------------------------------------------------------------------------
From: Tomi Valkeinen <hidden> Date: 2015-01-15 11:42:52
On 14/01/15 07:24, David Ung wrote:
quoted
-----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:
quoted
quoted
quoted
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
From: Tomi Valkeinen <hidden> Date: 2015-01-15 11:56:58
On 13/01/15 13:19, Tomi Valkeinen wrote:
On 29/12/14 23:51, David Ung wrote:
quoted
quoted
quoted
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 <redacted>
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 <redacted>
---
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(-)
From: Dan Carpenter <hidden> Date: 2015-01-15 12:26:35
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
From: Tomi Valkeinen <hidden> Date: 2015-01-15 12:39:01
On 15/01/15 14:26, Dan Carpenter wrote:
On Thu, Jan 15, 2015 at 01:56:58PM +0200, Tomi Valkeinen wrote:
quoted
Here's a fix for the cea_array size. Does it silence the static checker?
No.
quoted
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
From: Tomi Valkeinen <hidden> Date: 2015-01-15 12:52:29
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 <redacted>
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 <redacted>
Reported-by: Dan Carpenter <redacted>
---
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(-)
From: Dan Carpenter <hidden> Date: 2015-07-23 12:39:56
This was never merged.
regards,
dan carpenter
On Thu, Jan 15, 2015 at 02:52:29PM +0200, Tomi Valkeinen wrote:
quoted hunk
On 15/01/15 14:43, Dan Carpenter wrote:
quoted
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 <redacted>
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 <redacted>
Reported-by: Dan Carpenter <redacted>
---
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(-)