| Message ID | 20141226172657.GA14762@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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])); -- 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
> > 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. ----------------------------------------------------------------------------------- -- 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
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
> -----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. ----------------------------------------------------------------------------------- -- 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
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
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]));
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> -- 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