| Message ID | 5F9BE974.3040806@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/display/exynos4210_fimd: Fix potential NULL pointer dereference | expand |
On Fri, 30 Oct 2020 at 10:23, AlexChen <alex.chen@huawei.com> wrote: > > In exynos4210_fimd_update(), the pointer s is dereferenced before > being check if it is valid, which may lead to NULL pointer dereference. > So move the assignment to global_width after checking that the s is valid > > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: Alex Chen <alex.chen@huawei.com> > --- > hw/display/exynos4210_fimd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c > index 4c16e1f5a0..a1179d2f89 100644 > --- a/hw/display/exynos4210_fimd.c > +++ b/hw/display/exynos4210_fimd.c > @@ -1275,12 +1275,12 @@ static void exynos4210_fimd_update(void *opaque) > bool blend = false; > uint8_t *host_fb_addr; > bool is_dirty = false; > - const int global_width = (s->vidtcon[2] & FIMD_VIDTCON2_SIZE_MASK) + 1; > > if (!s || !s->console || !s->enabled || > surface_bits_per_pixel(qemu_console_surface(s->console)) == 0) { > return; > } > + const int global_width = (s->vidtcon[2] & FIMD_VIDTCON2_SIZE_MASK) + 1; > exynos4210_update_resolution(s); > surface = qemu_console_surface(s->console); Yep, this is a bug, but QEMU's coding style doesn't permit variable declarations in the middle of functions. You need to split the declaration (which stays where it is) and the initialization (which can moved down below the !s test. thanks -- PMM
On 2020/10/30 22:28, Peter Maydell wrote: > On Fri, 30 Oct 2020 at 10:23, AlexChen <alex.chen@huawei.com> wrote: >> >> In exynos4210_fimd_update(), the pointer s is dereferenced before >> being check if it is valid, which may lead to NULL pointer dereference. >> So move the assignment to global_width after checking that the s is valid >> >> Reported-by: Euler Robot <euler.robot@huawei.com> >> Signed-off-by: Alex Chen <alex.chen@huawei.com> >> --- >> hw/display/exynos4210_fimd.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c >> index 4c16e1f5a0..a1179d2f89 100644 >> --- a/hw/display/exynos4210_fimd.c >> +++ b/hw/display/exynos4210_fimd.c >> @@ -1275,12 +1275,12 @@ static void exynos4210_fimd_update(void *opaque) >> bool blend = false; >> uint8_t *host_fb_addr; >> bool is_dirty = false; >> - const int global_width = (s->vidtcon[2] & FIMD_VIDTCON2_SIZE_MASK) + 1; >> >> if (!s || !s->console || !s->enabled || >> surface_bits_per_pixel(qemu_console_surface(s->console)) == 0) { >> return; >> } >> + const int global_width = (s->vidtcon[2] & FIMD_VIDTCON2_SIZE_MASK) + 1; >> exynos4210_update_resolution(s); >> surface = qemu_console_surface(s->console); > > Yep, this is a bug, but QEMU's coding style doesn't permit > variable declarations in the middle of functions. You need > to split the declaration (which stays where it is) and the > initialization (which can moved down below the !s test. > Thans for your review. I have also considered this modification to be incompatible with the QEMU's coding style. But the type of global_width is const int which cannot be assigned once they are defined. Could we define the global_width as int, such as this modification: diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c index 4c16e1f5a0..34a960a976 100644 --- a/hw/display/exynos4210_fimd.c +++ b/hw/display/exynos4210_fimd.c @@ -1275,12 +1275,14 @@ static void exynos4210_fimd_update(void *opaque) bool blend = false; uint8_t *host_fb_addr; bool is_dirty = false; - const int global_width = (s->vidtcon[2] & FIMD_VIDTCON2_SIZE_MASK) + 1; + int global_width; if (!s || !s->console || !s->enabled || surface_bits_per_pixel(qemu_console_surface(s->console)) == 0) { return; } + + global_width = (s->vidtcon[2] & FIMD_VIDTCON2_SIZE_MASK) + 1; exynos4210_update_resolution(s); surface = qemu_console_surface(s->console);
On Sat, 31 Oct 2020 at 02:57, AlexChen <alex.chen@huawei.com> wrote: > > On 2020/10/30 22:28, Peter Maydell wrote: > > On Fri, 30 Oct 2020 at 10:23, AlexChen <alex.chen@huawei.com> wrote: > >> > >> In exynos4210_fimd_update(), the pointer s is dereferenced before > >> being check if it is valid, which may lead to NULL pointer dereference. > >> So move the assignment to global_width after checking that the s is valid > >> > >> Reported-by: Euler Robot <euler.robot@huawei.com> > >> Signed-off-by: Alex Chen <alex.chen@huawei.com> > >> --- > >> hw/display/exynos4210_fimd.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c > >> index 4c16e1f5a0..a1179d2f89 100644 > >> --- a/hw/display/exynos4210_fimd.c > >> +++ b/hw/display/exynos4210_fimd.c > >> @@ -1275,12 +1275,12 @@ static void exynos4210_fimd_update(void *opaque) > >> bool blend = false; > >> uint8_t *host_fb_addr; > >> bool is_dirty = false; > >> - const int global_width = (s->vidtcon[2] & FIMD_VIDTCON2_SIZE_MASK) + 1; > >> > >> if (!s || !s->console || !s->enabled || > >> surface_bits_per_pixel(qemu_console_surface(s->console)) == 0) { > >> return; > >> } > >> + const int global_width = (s->vidtcon[2] & FIMD_VIDTCON2_SIZE_MASK) + 1; > >> exynos4210_update_resolution(s); > >> surface = qemu_console_surface(s->console); > > > > Yep, this is a bug, but QEMU's coding style doesn't permit > > variable declarations in the middle of functions. You need > > to split the declaration (which stays where it is) and the > > initialization (which can moved down below the !s test. > > > Thans for your review. I have also considered this modification to be incompatible with > the QEMU's coding style. But the type of global_width is const int which cannot be > assigned once they are defined. > Could we define the global_width as int, such as this modification: > > diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c > index 4c16e1f5a0..34a960a976 100644 > --- a/hw/display/exynos4210_fimd.c > +++ b/hw/display/exynos4210_fimd.c > @@ -1275,12 +1275,14 @@ static void exynos4210_fimd_update(void *opaque) > bool blend = false; > uint8_t *host_fb_addr; > bool is_dirty = false; > - const int global_width = (s->vidtcon[2] & FIMD_VIDTCON2_SIZE_MASK) + 1; > + int global_width; > > if (!s || !s->console || !s->enabled || > surface_bits_per_pixel(qemu_console_surface(s->console)) == 0) { > return; > } > + > + global_width = (s->vidtcon[2] & FIMD_VIDTCON2_SIZE_MASK) + 1; > exynos4210_update_resolution(s); > surface = qemu_console_surface(s->console); Yes, I think that would be fine. thanks -- PMM
diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c index 4c16e1f5a0..a1179d2f89 100644 --- a/hw/display/exynos4210_fimd.c +++ b/hw/display/exynos4210_fimd.c @@ -1275,12 +1275,12 @@ static void exynos4210_fimd_update(void *opaque) bool blend = false; uint8_t *host_fb_addr; bool is_dirty = false; - const int global_width = (s->vidtcon[2] & FIMD_VIDTCON2_SIZE_MASK) + 1; if (!s || !s->console || !s->enabled || surface_bits_per_pixel(qemu_console_surface(s->console)) == 0) { return; } + const int global_width = (s->vidtcon[2] & FIMD_VIDTCON2_SIZE_MASK) + 1; exynos4210_update_resolution(s); surface = qemu_console_surface(s->console);
In exynos4210_fimd_update(), the pointer s is dereferenced before being check if it is valid, which may lead to NULL pointer dereference. So move the assignment to global_width after checking that the s is valid Reported-by: Euler Robot <euler.robot@huawei.com> Signed-off-by: Alex Chen <alex.chen@huawei.com> --- hw/display/exynos4210_fimd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)