All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] hw/display/exynos4210_fimd: Fix potential NULL pointer dereference
@ 2020-10-30 10:22 ` AlexChen
  0 siblings, 0 replies; 8+ messages in thread
From: AlexChen @ 2020-10-30 10:22 UTC (permalink / raw)
  To: i.mitsyanko, peter.maydell; +Cc: qemu-arm, QEMU, qemu-trivial

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);

-- 
2.19.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH] hw/display/exynos4210_fimd: Fix potential NULL pointer dereference
@ 2020-10-30 10:22 ` AlexChen
  0 siblings, 0 replies; 8+ messages in thread
From: AlexChen @ 2020-10-30 10:22 UTC (permalink / raw)
  To: i.mitsyanko, peter.maydell; +Cc: qemu-trivial, qemu-arm, QEMU

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);

-- 
2.19.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH] hw/display/exynos4210_fimd: Fix potential NULL pointer dereference
  2020-10-30 10:22 ` AlexChen
@ 2020-10-30 14:28   ` Peter Maydell
  -1 siblings, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2020-10-30 14:28 UTC (permalink / raw)
  To: AlexChen; +Cc: Igor Mitsyanko, qemu-arm, QEMU, QEMU Trivial

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


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] hw/display/exynos4210_fimd: Fix potential NULL pointer dereference
@ 2020-10-30 14:28   ` Peter Maydell
  0 siblings, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2020-10-30 14:28 UTC (permalink / raw)
  To: AlexChen; +Cc: Igor Mitsyanko, QEMU Trivial, qemu-arm, QEMU

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

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] hw/display/exynos4210_fimd: Fix potential NULL pointer dereference
  2020-10-30 14:28   ` Peter Maydell
@ 2020-10-31  2:57     ` AlexChen
  -1 siblings, 0 replies; 8+ messages in thread
From: AlexChen @ 2020-10-31  2:57 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Igor Mitsyanko, qemu-arm, QEMU, QEMU Trivial

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);



^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH] hw/display/exynos4210_fimd: Fix potential NULL pointer dereference
@ 2020-10-31  2:57     ` AlexChen
  0 siblings, 0 replies; 8+ messages in thread
From: AlexChen @ 2020-10-31  2:57 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Igor Mitsyanko, QEMU Trivial, qemu-arm, QEMU

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);


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH] hw/display/exynos4210_fimd: Fix potential NULL pointer dereference
  2020-10-31  2:57     ` AlexChen
@ 2020-10-31  9:47       ` Peter Maydell
  -1 siblings, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2020-10-31  9:47 UTC (permalink / raw)
  To: AlexChen; +Cc: Igor Mitsyanko, qemu-arm, QEMU, QEMU Trivial

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


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] hw/display/exynos4210_fimd: Fix potential NULL pointer dereference
@ 2020-10-31  9:47       ` Peter Maydell
  0 siblings, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2020-10-31  9:47 UTC (permalink / raw)
  To: AlexChen; +Cc: Igor Mitsyanko, QEMU Trivial, qemu-arm, QEMU

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

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-10-31  9:48 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-30 10:22 [PATCH] hw/display/exynos4210_fimd: Fix potential NULL pointer dereference AlexChen
2020-10-30 10:22 ` AlexChen
2020-10-30 14:28 ` Peter Maydell
2020-10-30 14:28   ` Peter Maydell
2020-10-31  2:57   ` AlexChen
2020-10-31  2:57     ` AlexChen
2020-10-31  9:47     ` Peter Maydell
2020-10-31  9:47       ` Peter Maydell

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.