1. Patchew
  2. QEMU
  3. [PULL 00/92] Misc patches for 2020-09-24
  4. View patch

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

Paolo Bonzini posted 92 patches 5 years, 4 months ago
Maintainers: Fam Zheng <fam@euphon.net>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, Thomas Huth <thuth@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Markus Armbruster <armbru@redhat.com>, Bandan Das <bsd@redhat.com>, Marcelo Tosatti <mtosatti@redhat.com>, Michael Roth <mdroth@linux.vnet.ibm.com>, Peter Maydell <peter.maydell@linaro.org>, Jason Wang <jasowang@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Sergio Lopez <slp@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Hannes Reinecke <hare@suse.com>, Stefan Weil <sw@weilnetz.de>, Cleber Rosa <crosa@redhat.com>, Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>, David Hildenbrand <david@redhat.com>, Aurelien Jarno <aurelien@aurel32.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Eric Blake <eblake@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Alexander Bulekov <alxndr@bu.edu>, "Michael S. Tsirkin" <mst@redhat.com>, David Gibson <david@gibson.dropbear.id.au>, Max Reitz <mreitz@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Sunil Muthuswamy <sunilmut@microsoft.com>, Richard Henderson <rth@twiddle.net>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Kevin Wolf <kwolf@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Halil Pasic <pasic@linux.ibm.com>
There is a newer version of this series
[PULL 19/92] hw/char/serial: Assert serial_ioport_read/write offset fits 8 bytes
Posted by Paolo Bonzini 5 years, 4 months ago 
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:
@@ -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:
-- 
2.26.2
Re: [PULL 19/92] hw/char/serial: Assert serial_ioport_read/write offset fits 8 bytes
Posted by Peter Maydell 5 years, 2 months ago 
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
Re: [PULL 19/92] hw/char/serial: Assert serial_ioport_read/write offset fits 8 bytes
Posted by Paolo Bonzini 5 years, 2 months ago 
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
Re: [PULL 19/92] hw/char/serial: Assert serial_ioport_read/write offset fits 8 bytes
Posted by Philippe Mathieu-Daudé 5 years, 2 months ago 
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
> 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.