* [PATCH] hw/display/bcm2835_fb.c: Initialize all fields of struct
@ 2020-06-28 19:54 Peter Maydell
2020-06-29 7:14 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 3+ messages in thread
From: Peter Maydell @ 2020-06-28 19:54 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Philippe Mathieu-Daudé, Andrew Baumann
In bcm2835_fb_mbox_push(), Coverity complains (CID 1429989) that we
pass a pointer to a local struct to another function without
initializing all its fields. This is a real bug:
bcm2835_fb_reconfigure() copies the whole of our new BCM2385FBConfig
struct into s->config, so any fields we don't initialize will corrupt
the state of the device.
Copy the two fields which we don't want to update (pixo and alpha)
from the existing config so we don't accidentally change them.
Fixes: cfb7ba983857e40e88
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Not sure why this wasn't a visible bug -- alpha isn't used,
but if pixo changes from zero to non-zero we flip from
RGB to BGR...
---
hw/display/bcm2835_fb.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index c6263808a27..7c0e5eef2d5 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -282,6 +282,10 @@ static void bcm2835_fb_mbox_push(BCM2835FBState *s, uint32_t value)
newconf.base = s->vcram_base | (value & 0xc0000000);
newconf.base += BCM2835_FB_OFFSET;
+ /* Copy fields which we don't want to change from the existing config */
+ newconf.pixo = s->config.pixo;
+ newconf.alpha = s->config.alpha;
+
bcm2835_fb_validate_config(&newconf);
pitch = bcm2835_fb_get_pitch(&newconf);
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] hw/display/bcm2835_fb.c: Initialize all fields of struct
2020-06-28 19:54 [PATCH] hw/display/bcm2835_fb.c: Initialize all fields of struct Peter Maydell
@ 2020-06-29 7:14 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 3+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-29 7:14 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Andrew Baumann
On 6/28/20 9:54 PM, Peter Maydell wrote:
> In bcm2835_fb_mbox_push(), Coverity complains (CID 1429989) that we
> pass a pointer to a local struct to another function without
> initializing all its fields. This is a real bug:
> bcm2835_fb_reconfigure() copies the whole of our new BCM2385FBConfig
> struct into s->config, so any fields we don't initialize will corrupt
> the state of the device.
>
> Copy the two fields which we don't want to update (pixo and alpha)
> from the existing config so we don't accidentally change them.
>
> Fixes: cfb7ba983857e40e88
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Not sure why this wasn't a visible bug -- alpha isn't used,
> but if pixo changes from zero to non-zero we flip from
> RGB to BGR...
> ---
> hw/display/bcm2835_fb.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
> index c6263808a27..7c0e5eef2d5 100644
> --- a/hw/display/bcm2835_fb.c
> +++ b/hw/display/bcm2835_fb.c
> @@ -282,6 +282,10 @@ static void bcm2835_fb_mbox_push(BCM2835FBState *s, uint32_t value)
> newconf.base = s->vcram_base | (value & 0xc0000000);
> newconf.base += BCM2835_FB_OFFSET;
>
> + /* Copy fields which we don't want to change from the existing config */
> + newconf.pixo = s->config.pixo;
> + newconf.alpha = s->config.alpha;
> +
> bcm2835_fb_validate_config(&newconf);
>
> pitch = bcm2835_fb_get_pitch(&newconf);
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] hw/display/bcm2835_fb.c: Initialize all fields of struct
2020-10-12 15:33 [PATCH 00/10] target/arm: Various v8.1M minor features Peter Maydell
@ 2020-10-12 15:33 ` Peter Maydell
0 siblings, 0 replies; 3+ messages in thread
From: Peter Maydell @ 2020-10-12 15:33 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: Richard Henderson
In bcm2835_fb_mbox_push(), Coverity complains (CID 1429989) that we
pass a pointer to a local struct to another function without
initializing all its fields. This is a real bug:
bcm2835_fb_reconfigure() copies the whole of our new BCM2385FBConfig
struct into s->config, so any fields we don't initialize will corrupt
the state of the device.
Copy the two fields which we don't want to update (pixo and alpha)
from the existing config so we don't accidentally change them.
Fixes: cfb7ba983857e40e88
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Not sure why this wasn't a visible bug -- alpha isn't used,
but if pixo changes from zero to non-zero we flip from
RGB to BGR...
---
hw/display/bcm2835_fb.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
index c6263808a27..7c0e5eef2d5 100644
--- a/hw/display/bcm2835_fb.c
+++ b/hw/display/bcm2835_fb.c
@@ -282,6 +282,10 @@ static void bcm2835_fb_mbox_push(BCM2835FBState *s, uint32_t value)
newconf.base = s->vcram_base | (value & 0xc0000000);
newconf.base += BCM2835_FB_OFFSET;
+ /* Copy fields which we don't want to change from the existing config */
+ newconf.pixo = s->config.pixo;
+ newconf.alpha = s->config.alpha;
+
bcm2835_fb_validate_config(&newconf);
pitch = bcm2835_fb_get_pitch(&newconf);
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-12 15:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-28 19:54 [PATCH] hw/display/bcm2835_fb.c: Initialize all fields of struct Peter Maydell
2020-06-29 7:14 ` Philippe Mathieu-Daudé
-- strict thread matches above, loose matches on Subject: below --
2020-10-12 15:33 [PATCH 00/10] target/arm: Various v8.1M minor features Peter Maydell
2020-10-12 15:33 ` [PATCH] hw/display/bcm2835_fb.c: Initialize all fields of struct Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).