* Re: [PATCH 12/13] drm/i2c/tda998x: Fix signed overflow issue
@ 2014-04-05 11:29 Jean-Francois Moine
2014-04-05 16:25 ` [PATCH] " Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Jean-Francois Moine @ 2014-04-05 11:29 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Russell King, dri-devel
On Sat Apr 5 02:45:01 PDT 2014,
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> This is C standard hair-splitting, but afaict
> - sum will be promoted to signed int in computation since
> uint8_t fits
> - signed overflow is undefined.
[snip]
> drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 48af5cac1902..ae2754760d77 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -568,7 +568,7 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
>
> static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
> {
> - uint8_t sum = 0;
> + unsigned sum = 0;
>
> while (bytes--)
> sum += *buf++;
This function may be simplified by:
--- tda998x_drv.c~
+++ tda998x_drv.c
@@ -568,11 +568,11 @@
static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
{
- uint8_t sum = 0;
+ int sum = 0;
while (bytes--)
- sum += *buf++;
- return (255 - sum) + 1;
+ sum -= *buf++;
+ return sum;
}
#define HB(x) (x)
and the same may be done in hdmi.c:
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 9e758a8..b6c9030 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -31,14 +31,14 @@
static void hdmi_infoframe_checksum(void *buffer, size_t size)
{
u8 *ptr = buffer;
- u8 csum = 0;
+ int csum = 0;
size_t i;
/* compute checksum */
for (i = 0; i < size; i++)
- csum += ptr[i];
+ csum -= ptr[i];
- ptr[3] = 256 - csum;
+ ptr[3] = csum;
}
/**
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH] drm/i2c/tda998x: Fix signed overflow issue
2014-04-05 11:29 [PATCH 12/13] drm/i2c/tda998x: Fix signed overflow issue Jean-Francois Moine
@ 2014-04-05 16:25 ` Daniel Vetter
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2014-04-05 16:25 UTC (permalink / raw)
To: DRI Development; +Cc: Dave Jones, Russell King, Daniel Vetter
This is C standard hair-splitting, but afaict
- sum will be promoted to signed int in computation since
uint8_t fits
- signed overflow is undefined.
No we need to add up an awful lot of bytes to actually make it
overflow. But I guess the real risk is gcc spotting this and going
bananas. Fix this by simply using unsigned in to force all computations
to use the well-defined unsigned behaviour.
Spotted by coverity.
v2: Simplify the entire computation as suggested by Jean.
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Jean-Francois Moine <moinejf@free.fr>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i2c/tda998x_drv.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 48af5cac1902..240c331405b9 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -568,11 +568,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
{
- uint8_t sum = 0;
+ int sum = 0;
while (bytes--)
- sum += *buf++;
- return (255 - sum) + 1;
+ sum -= *buf++;
+ return sum;
}
#define HB(x) (x)
--
1.8.5.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 00/13] coverity
@ 2014-04-05 9:44 Daniel Vetter
2014-04-05 9:45 ` [PATCH 12/13] drm/i2c/tda998x: Fix signed overflow issue Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Vetter @ 2014-04-05 9:44 UTC (permalink / raw)
To: DRI Development; +Cc: Dave Jones, Daniel Vetter
Hi all,
Rainy w/e here and I got a bit bored, so looked at coverity again. I've closed
all outstanding issues in drivers/gpu now either as false positives or fixed in
this series expect for vmwgfx/ttm stuff (not enough clue) and one insane savage
init issue (no desire to wake dragons today).
Cheers, Daniel
Daniel Vetter (13):
drm/mgag200: Remove unnecessary NULL check in bo_unref
drm/mgag200: Remove unecessary NULL check in gem_free
drm/cirrus: Remove unnecessary NULL check in bo_unref
drm/cirrus: Remove unecessary NULL check in gem_free
drm/ast: Remove unnecessary NULL check in bo_unref
drm/mgag200: Remove unecessary NULL check in gem_free
drm/via: Remove unecessary NULL check
drm/ast: Remove dead code from cbr_scan2
drm/udl: Initialize ret in udl_driver_load
drm/bochs: Remove unnecessary NULL check in bo_unref
drm/bochs: Remove unecessary NULL check in gem_free
drm/i2c/tda998x: Fix signed overflow issue
drm: Fix error handling in drm_master_create
drivers/gpu/drm/ast/ast_main.c | 7 ++-----
drivers/gpu/drm/ast/ast_post.c | 2 --
drivers/gpu/drm/bochs/bochs_mm.c | 6 +-----
drivers/gpu/drm/cirrus/cirrus_main.c | 6 +-----
drivers/gpu/drm/drm_stub.c | 5 ++++-
drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
drivers/gpu/drm/mgag200/mgag200_main.c | 6 +-----
drivers/gpu/drm/udl/udl_main.c | 1 +
drivers/gpu/drm/via/via_mm.c | 2 +-
9 files changed, 12 insertions(+), 25 deletions(-)
--
1.8.5.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 12/13] drm/i2c/tda998x: Fix signed overflow issue
2014-04-05 9:44 [PATCH 00/13] coverity Daniel Vetter
@ 2014-04-05 9:45 ` Daniel Vetter
2014-04-07 15:31 ` Ian Romanick
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Vetter @ 2014-04-05 9:45 UTC (permalink / raw)
To: DRI Development; +Cc: Dave Jones, Russell King, Daniel Vetter
This is C standard hair-splitting, but afaict
- sum will be promoted to signed int in computation since
uint8_t fits
- signed overflow is undefined.
No we need to add up an awful lot of bytes to actually make it
overflow. But I guess the real risk is gcc spotting this and going
bananas. Fix this by simply using unsigned in to force all computations
to use the well-defined unsigned behaviour.
Spotted by coverity.
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Jean-Francois Moine <moinejf@free.fr>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 48af5cac1902..ae2754760d77 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -568,7 +568,7 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
{
- uint8_t sum = 0;
+ unsigned sum = 0;
while (bytes--)
sum += *buf++;
--
1.8.5.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 12/13] drm/i2c/tda998x: Fix signed overflow issue
2014-04-05 9:45 ` [PATCH 12/13] drm/i2c/tda998x: Fix signed overflow issue Daniel Vetter
@ 2014-04-07 15:31 ` Ian Romanick
0 siblings, 0 replies; 4+ messages in thread
From: Ian Romanick @ 2014-04-07 15:31 UTC (permalink / raw)
To: Daniel Vetter, DRI Development; +Cc: Dave Jones, Russell King
On 04/05/2014 02:45 AM, Daniel Vetter wrote:
> This is C standard hair-splitting, but afaict
> - sum will be promoted to signed int in computation since
> uint8_t fits
> - signed overflow is undefined.
>
> No we need to add up an awful lot of bytes to actually make it
^^
Now
> overflow. But I guess the real risk is gcc spotting this and going
> bananas. Fix this by simply using unsigned in to force all computations
> to use the well-defined unsigned behaviour.
Seems reasonable... it also seems impossible (ha!) to break anything.
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
> Spotted by coverity.
>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Jean-Francois Moine <moinejf@free.fr>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 48af5cac1902..ae2754760d77 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -568,7 +568,7 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
>
> static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
> {
> - uint8_t sum = 0;
> + unsigned sum = 0;
>
> while (bytes--)
> sum += *buf++;
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-04-07 15:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-05 11:29 [PATCH 12/13] drm/i2c/tda998x: Fix signed overflow issue Jean-Francois Moine
2014-04-05 16:25 ` [PATCH] " Daniel Vetter
-- strict thread matches above, loose matches on Subject: below --
2014-04-05 9:44 [PATCH 00/13] coverity Daniel Vetter
2014-04-05 9:45 ` [PATCH 12/13] drm/i2c/tda998x: Fix signed overflow issue Daniel Vetter
2014-04-07 15:31 ` Ian Romanick
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.