diff mbox series

[PULL,19/92] hw/char/serial: Assert serial_ioport_read/write offset fits 8 bytes

Message ID 20200924092314.1722645-20-pbonzini@redhat.com
State New
Headers show
Series Misc patches for 2020-09-24 | expand

Commit Message

Paolo Bonzini Sept. 24, 2020, 9:22 a.m. UTC
From: Philippe Mathieu-Daudé <f4bug@amsat.org>

The serial device has 8 registers, each 8-bit. The MemoryRegionOps
'serial_io_ops' is initialized with max_access_size=1, and all
memory_region_init_io() callers correctly set the region size to
8 bytes:
- serial_io_realize
- serial_isa_realizefn
- serial_pci_realize
- multi_serial_pci_realize

It is safe to assert the offset argument of serial_ioport_read()
and serial_ioport_write() is always less than 8.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20200907015535.827885-2-f4bug@amsat.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Peter Maydell Nov. 18, 2020, 3:40 p.m. UTC | #1
On Thu, 24 Sep 2020 at 10:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>

> From: Philippe Mathieu-Daudé <f4bug@amsat.org>

>

> The serial device has 8 registers, each 8-bit. The MemoryRegionOps

> 'serial_io_ops' is initialized with max_access_size=1, and all

> memory_region_init_io() callers correctly set the region size to

> 8 bytes:

> - serial_io_realize

> - serial_isa_realizefn

> - serial_pci_realize

> - multi_serial_pci_realize

>

> It is safe to assert the offset argument of serial_ioport_read()

> and serial_ioport_write() is always less than 8.

>

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

> Message-Id: <20200907015535.827885-2-f4bug@amsat.org>

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> ---

>  hw/char/serial.c | 4 ++--

>  1 file changed, 2 insertions(+), 2 deletions(-)

>

> diff --git a/hw/char/serial.c b/hw/char/serial.c

> index fd80ae5592..840da89de7 100644

> --- a/hw/char/serial.c

> +++ b/hw/char/serial.c

> @@ -344,7 +344,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,

>  {

>      SerialState *s = opaque;

>

> -    addr &= 7;

> +    assert(size == 1 && addr < 8);

>      trace_serial_ioport_write(addr, val);

>      switch(addr) {

>      default:


Bug report https://bugs.launchpad.net/qemu/+bug/1904331
points out that the addition of this assert() makes obvious
that either the assert is wrong or some code later in the
function which is looking at size must be dead:
            if (size == 1) {
                s->divider = (s->divider & 0xff00) | val;
            } else {
                s->divider = val;
            }

Presumably it's the if() that should be fixed ?

thanks
-- PMM
Paolo Bonzini Nov. 18, 2020, 5:08 p.m. UTC | #2
On 18/11/20 16:40, Peter Maydell wrote:
> On Thu, 24 Sep 2020 at 10:40, Paolo Bonzini <pbonzini@redhat.com> wrote:

>>

>> From: Philippe Mathieu-Daudé <f4bug@amsat.org>

>>

>> The serial device has 8 registers, each 8-bit. The MemoryRegionOps

>> 'serial_io_ops' is initialized with max_access_size=1, and all

>> memory_region_init_io() callers correctly set the region size to

>> 8 bytes:

>> - serial_io_realize

>> - serial_isa_realizefn

>> - serial_pci_realize

>> - multi_serial_pci_realize

>>

>> It is safe to assert the offset argument of serial_ioport_read()

>> and serial_ioport_write() is always less than 8.

>>

>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

>> Message-Id: <20200907015535.827885-2-f4bug@amsat.org>

>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

>> ---

>>   hw/char/serial.c | 4 ++--

>>   1 file changed, 2 insertions(+), 2 deletions(-)

>>

>> diff --git a/hw/char/serial.c b/hw/char/serial.c

>> index fd80ae5592..840da89de7 100644

>> --- a/hw/char/serial.c

>> +++ b/hw/char/serial.c

>> @@ -344,7 +344,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,

>>   {

>>       SerialState *s = opaque;

>>

>> -    addr &= 7;

>> +    assert(size == 1 && addr < 8);

>>       trace_serial_ioport_write(addr, val);

>>       switch(addr) {

>>       default:

> 

> Bug report https://bugs.launchpad.net/qemu/+bug/1904331

> points out that the addition of this assert() makes obvious

> that either the assert is wrong or some code later in the

> function which is looking at size must be dead:

>              if (size == 1) {

>                  s->divider = (s->divider & 0xff00) | val;

>              } else {

>                  s->divider = val;

>              }

> 

> Presumably it's the if() that should be fixed ?


It can be dropped, because serial_io_ops has

     .impl = {
         .min_access_size = 1,
         .max_access_size = 1,
     },

Therefore, a 16-bit write to addr==0 is automatically split into an 
8-byte write to addr==0 and one to addr=1.  Together, the two set the 
full 16 bits of s->divider.

Thanks,

Paolo
Philippe Mathieu-Daudé Nov. 18, 2020, 6:37 p.m. UTC | #3
On 11/18/20 6:08 PM, Paolo Bonzini wrote:
> On 18/11/20 16:40, Peter Maydell wrote:

>> On Thu, 24 Sep 2020 at 10:40, Paolo Bonzini <pbonzini@redhat.com> wrote:

>>>

>>> From: Philippe Mathieu-Daudé <f4bug@amsat.org>

>>>

>>> The serial device has 8 registers, each 8-bit. The MemoryRegionOps

>>> 'serial_io_ops' is initialized with max_access_size=1, and all

>>> memory_region_init_io() callers correctly set the region size to

>>> 8 bytes:

>>> - serial_io_realize

>>> - serial_isa_realizefn

>>> - serial_pci_realize

>>> - multi_serial_pci_realize

>>>

>>> It is safe to assert the offset argument of serial_ioport_read()

>>> and serial_ioport_write() is always less than 8.

>>>

>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

>>> Message-Id: <20200907015535.827885-2-f4bug@amsat.org>

>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

>>> ---

>>>   hw/char/serial.c | 4 ++--

>>>   1 file changed, 2 insertions(+), 2 deletions(-)

>>>

>>> diff --git a/hw/char/serial.c b/hw/char/serial.c

>>> index fd80ae5592..840da89de7 100644

>>> --- a/hw/char/serial.c

>>> +++ b/hw/char/serial.c

>>> @@ -344,7 +344,7 @@ static void serial_ioport_write(void *opaque,

>>> hwaddr addr, uint64_t val,

>>>   {

>>>       SerialState *s = opaque;

>>>

>>> -    addr &= 7;

>>> +    assert(size == 1 && addr < 8);

>>>       trace_serial_ioport_write(addr, val);

>>>       switch(addr) {

>>>       default:

>>

>> Bug report https://bugs.launchpad.net/qemu/+bug/1904331

>> points out that the addition of this assert() makes obvious

>> that either the assert is wrong or some code later in the

>> function which is looking at size must be dead:

>>              if (size == 1) {

>>                  s->divider = (s->divider & 0xff00) | val;

>>              } else {

>>                  s->divider = val;

>>              }

>>

>> Presumably it's the if() that should be fixed ?

> 

> It can be dropped, because serial_io_ops has

> 

>     .impl = {

>         .min_access_size = 1,

>         .max_access_size = 1,

>     },

> 

> Therefore, a 16-bit write to addr==0 is automatically split into an

> 8-byte write to addr==0 and one to addr=1.  Together, the two set the

> full 16 bits of s->divider.


Since commit 5ec3a23e6c8 ("serial: convert PIO to new memory api
read/write") =)

> 

> Thanks,

> 

> Paolo

> 

>
diff mbox series

Patch

diff --git a/hw/char/serial.c b/hw/char/serial.c
index fd80ae5592..840da89de7 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -344,7 +344,7 @@  static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
 {
     SerialState *s = opaque;
 
-    addr &= 7;
+    assert(size == 1 && addr < 8);
     trace_serial_ioport_write(addr, val);
     switch(addr) {
     default:
@@ -485,7 +485,7 @@  static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
     SerialState *s = opaque;
     uint32_t ret;
 
-    addr &= 7;
+    assert(size == 1 && addr < 8);
     switch(addr) {
     default:
     case 0: