From: Prasad J Pandit <pjp@fedoraproject.org>

Define skeleton 'power_mem_read' routine. Avoid NULL dereference.

Reported-by: Fakhri Zulkifli <mohdfakhrizulkifli@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/sparc64/sun4u.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index f76b19e4e9..cbdb2bb411 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -214,6 +214,11 @@ typedef struct PowerDevice {
 } PowerDevice;
 
 /* Power */
+static uint64_t power_mem_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return 0xffffffff;
+}
+
 static void power_mem_write(void *opaque, hwaddr addr,
                             uint64_t val, unsigned size)
 {
@@ -224,6 +229,7 @@ static void power_mem_write(void *opaque, hwaddr addr,
 }
 
 static const MemoryRegionOps power_mem_ops = {
+    .read = power_mem_read,
     .write = power_mem_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .valid = {
-- 
2.20.1

On 03/01/2019 09:37, P J P wrote:

> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> Define skeleton 'power_mem_read' routine. Avoid NULL dereference.
> 
> Reported-by: Fakhri Zulkifli <mohdfakhrizulkifli@gmail.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/sparc64/sun4u.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index f76b19e4e9..cbdb2bb411 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -214,6 +214,11 @@ typedef struct PowerDevice {
>  } PowerDevice;
>  
>  /* Power */
> +static uint64_t power_mem_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return 0xffffffff;
> +}
> +
>  static void power_mem_write(void *opaque, hwaddr addr,
>                              uint64_t val, unsigned size)
>  {
> @@ -224,6 +229,7 @@ static void power_mem_write(void *opaque, hwaddr addr,
>  }
>  
>  static const MemoryRegionOps power_mem_ops = {
> +    .read = power_mem_read,
>      .write = power_mem_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>      .valid = {

Certainly the addition of the missing read function is valid, although I see that
you've defaulted reads to returning 0xffffffff - can you point me towards the source
that indicates that this behaviour matches that of real Ultra-5 hardware?


ATB,

Mark.

  Hello Mark,

+-- On Thu, 3 Jan 2019, Mark Cave-Ayland wrote --+
| >  /* Power */
| > +static uint64_t power_mem_read(void *opaque, hwaddr addr, unsigned size)
| > +{
| > +    return 0xffffffff;
| > +}
| > +
| >  
| >  static const MemoryRegionOps power_mem_ops = {
| > +    .read = power_mem_read,
| >      .write = power_mem_write,
| >      .endianness = DEVICE_NATIVE_ENDIAN,
| >      .valid = {
| 
| Certainly the addition of the missing read function is valid, although I see that
| you've defaulted reads to returning 0xffffffff - can you point me towards the source
| that indicates that this behaviour matches that of real Ultra-5 hardware?

 -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02787.html

I haven't checked Ultra-5 h/w spec, return value was chosen similar to above 
one, thinking unimplemented .read need not return success.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

On 04/01/2019 05:47, P J P wrote:

>   Hello Mark,
> 
> +-- On Thu, 3 Jan 2019, Mark Cave-Ayland wrote --+
> | >  /* Power */
> | > +static uint64_t power_mem_read(void *opaque, hwaddr addr, unsigned size)
> | > +{
> | > +    return 0xffffffff;
> | > +}
> | > +
> | >  
> | >  static const MemoryRegionOps power_mem_ops = {
> | > +    .read = power_mem_read,
> | >      .write = power_mem_write,
> | >      .endianness = DEVICE_NATIVE_ENDIAN,
> | >      .valid = {
> | 
> | Certainly the addition of the missing read function is valid, although I see that
> | you've defaulted reads to returning 0xffffffff - can you point me towards the source
> | that indicates that this behaviour matches that of real Ultra-5 hardware?
> 
>  -> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02787.html
> 
> I haven't checked Ultra-5 h/w spec, return value was chosen similar to above 
> one, thinking unimplemented .read need not return success.
> 
> Thank you.

I asked someone with real Ultra-5 hardware to check this for me, and they've sent me
back the following output:

ok .properties
address                  fffb8000
button
interrupts               00000001
reg                      00000014 00724000 00000004
name                     power
ok fffb8000 l@ u.
0

Looks like the read value should be all 0s rather than all 1s. Can you resubmit the
patch with this change?


ATB,

Mark.

+-- On Fri, 4 Jan 2019, Mark Cave-Ayland wrote --+
| I asked someone with real Ultra-5 hardware to check this for me, and they've 
| sent me back the following output:
| 
| ok .properties
| address                  fffb8000
| button
| interrupts               00000001
| reg                      00000014 00724000 00000004
| name                     power
| ok fffb8000 l@ u.
| 0
| 
| Looks like the read value should be all 0s rather than all 1s. Can you resubmit the
| patch with this change?

Done. Sent patch v1.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Le ven. 4 janv. 2019 10:55, P J P <ppandit@redhat.com> a écrit :

> +-- On Fri, 4 Jan 2019, Mark Cave-Ayland wrote --+
> | I asked someone with real Ultra-5 hardware to check this for me, and
> they've
> | sent me back the following output:
> |
> | ok .properties
> | address                  fffb8000
> | button
> | interrupts               00000001
> | reg                      00000014 00724000 00000004
> | name                     power
> | ok fffb8000 l@ u.
> | 0
> |
> | Looks like the read value should be all 0s rather than all 1s. Can you
> resubmit the
> | patch with this change?
>
> Done. Sent patch v1.
>

The first patch is assumed to be v1, the second v2 and so on ;)

On 04/01/2019 09:53, P J P wrote:

> +-- On Fri, 4 Jan 2019, Mark Cave-Ayland wrote --+
> | I asked someone with real Ultra-5 hardware to check this for me, and they've 
> | sent me back the following output:
> | 
> | ok .properties
> | address                  fffb8000
> | button
> | interrupts               00000001
> | reg                      00000014 00724000 00000004
> | name                     power
> | ok fffb8000 l@ u.
> | 0
> | 
> | Looks like the read value should be all 0s rather than all 1s. Can you resubmit the
> | patch with this change?
> 
> Done. Sent patch v1.

Thanks. I've applied this to my qemu-sparc branch.


ATB,

Mark.