Hi,

This series convert the chardev::qemu_chr_write() to take unsigned
length argument. To do so I went through all caller and checked if
there are no negative value possible.

I'm having headaches with the Xen backend, talking with Marc-André
he suggested I ask help to the Xen maintainers.

Since the series is becoming quite big, I splitted it:
- part 1: convert qemu_chr_write()
- part 2: convert IOReadHandler

part 1 can be reviewed and merged without part 2.

The git diffstat is not huge, but there are various chardev subsystems
so many maintainers to ask for Ack.

v2: https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02396.html
v1: https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02200.html
    (from Prasad J Pandit)
Changes requested by Paolo:
https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02294.html

Please review,

Phil.

Philippe Mathieu-Daudé (25):
  chardev: Simplify IOWatchPoll::fd_can_read as a GSourceFunc
  chardev: Assert IOCanReadHandler can not be negative
  chardev/wctablet: Use unsigned type to hold unsigned value
  chardev: Let qemu_chr_be_can_write() return a size_t types
  gdbstub: Use size_t for strlen() return value
  gdbstub: Use size_t to hold GDBState::last_packet_len
  gdbstub: Let put_buffer() use size_t
  ui/gtk: Remove pointless cast
  vhost-user: Express sizeof with size_t
  usb-redir: Verify usbredirparser_write get called with positive count
  xen: Let xencons_send() take a 'size' argument
  xen: Let buffer_append() return the size consumed
  RFC xen: Let buffer_append() return a size_t
  virtio-serial: Let VirtIOSerialPortClass::have_data() use size_t
  spapr-vty: Let vty_putchars() use size_t
  tpm: Use size_t to hold sizes
  net/filter-mirror: Use size_t
  s390x/3270: Let insert_IAC_escape_char() use size_t
  s390/ebcdic: Use size_t to iterate over arrays
  s390x/sclp: Use a const variable to improve readability
  s390x/sclp: Use size_t in process_mdb()
  s390x/sclp: Let write_console_data() take a size_t
  hw/ipmi: Assert outlen > outpos
  chardev: Let qemu_chr_fe_write[_all] use size_t type argument
  chardev: Let qemu_chr_write[_all] use size_t

 chardev/baum.c                    |  6 +++---
 chardev/char-fd.c                 |  6 +++---
 chardev/char-fe.c                 |  4 ++--
 chardev/char-io.c                 |  6 +++---
 chardev/char-mux.c                |  3 ++-
 chardev/char-pty.c                |  8 ++++----
 chardev/char-socket.c             | 13 +++++++------
 chardev/char-udp.c                |  8 ++++----
 chardev/char-win.c                |  2 +-
 chardev/char.c                    | 15 +++++++++------
 chardev/msmouse.c                 |  4 ++--
 chardev/spice.c                   |  2 +-
 chardev/trace-events              |  2 +-
 chardev/wctablet.c                |  9 +++++----
 gdbstub.c                         |  8 ++++----
 hw/bt/hci-csr.c                   |  2 +-
 hw/char/sclpconsole-lm.c          | 12 +++++++-----
 hw/char/spapr_vty.c               |  2 +-
 hw/char/terminal3270.c            |  7 ++++---
 hw/char/virtio-console.c          |  2 +-
 hw/char/xen_console.c             | 24 +++++++++++++++---------
 hw/ipmi/ipmi_bmc_extern.c         |  3 ++-
 hw/tpm/tpm_emulator.c             |  7 ++++---
 hw/usb/redirect.c                 |  6 +++++-
 hw/virtio/vhost-user.c            | 14 ++++++++------
 include/chardev/char-fd.h         |  2 +-
 include/chardev/char-fe.h         |  4 ++--
 include/chardev/char-io.h         |  2 +-
 include/chardev/char.h            |  4 ++--
 include/hw/ppc/spapr_vio.h        |  2 +-
 include/hw/s390x/ebcdic.h         |  8 ++++----
 include/hw/virtio/virtio-serial.h |  2 +-
 include/sysemu/replay.h           |  2 +-
 net/filter-mirror.c               |  2 +-
 replay/replay-char.c              |  2 +-
 stubs/replay.c                    |  2 +-
 ui/console.c                      |  6 +++---
 ui/gtk.c                          |  2 +-
 38 files changed, 119 insertions(+), 96 deletions(-)

-- 
2.20.1

Hi

On Wed, Feb 20, 2019 at 2:02 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Hi,
>
> This series convert the chardev::qemu_chr_write() to take unsigned
> length argument. To do so I went through all caller and checked if
> there are no negative value possible.


Changing signedness is problematic and can easily introduce bugs that
are easy to miss during review.

I agree with Cornelia about idiomatic use of int. Changing "int" for
"size_t" isn't systematically a clear win.

Even Google C++ style recommends to avoid unsigned types "(except for
representing bitfields or modular arithmetic). Do not use an unsigned
type merely to assert that a variable is non-negative."
https://google.github.io/styleguide/cppguide.html#Integer_Types - see rationale

Since Paolo you suggested the change, could you give some convincing
arguments that it's worth taking the plunge?

thanks

>
> I'm having headaches with the Xen backend, talking with Marc-André
> he suggested I ask help to the Xen maintainers.
>
> Since the series is becoming quite big, I splitted it:
> - part 1: convert qemu_chr_write()
> - part 2: convert IOReadHandler
>
> part 1 can be reviewed and merged without part 2.
>
> The git diffstat is not huge, but there are various chardev subsystems
> so many maintainers to ask for Ack.
>
> v2: https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02396.html
> v1: https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02200.html
>     (from Prasad J Pandit)
> Changes requested by Paolo:
> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02294.html
>
> Please review,
>
> Phil.
>
> Philippe Mathieu-Daudé (25):
>   chardev: Simplify IOWatchPoll::fd_can_read as a GSourceFunc
>   chardev: Assert IOCanReadHandler can not be negative
>   chardev/wctablet: Use unsigned type to hold unsigned value
>   chardev: Let qemu_chr_be_can_write() return a size_t types
>   gdbstub: Use size_t for strlen() return value
>   gdbstub: Use size_t to hold GDBState::last_packet_len
>   gdbstub: Let put_buffer() use size_t
>   ui/gtk: Remove pointless cast
>   vhost-user: Express sizeof with size_t
>   usb-redir: Verify usbredirparser_write get called with positive count
>   xen: Let xencons_send() take a 'size' argument
>   xen: Let buffer_append() return the size consumed
>   RFC xen: Let buffer_append() return a size_t
>   virtio-serial: Let VirtIOSerialPortClass::have_data() use size_t
>   spapr-vty: Let vty_putchars() use size_t
>   tpm: Use size_t to hold sizes
>   net/filter-mirror: Use size_t
>   s390x/3270: Let insert_IAC_escape_char() use size_t
>   s390/ebcdic: Use size_t to iterate over arrays
>   s390x/sclp: Use a const variable to improve readability
>   s390x/sclp: Use size_t in process_mdb()
>   s390x/sclp: Let write_console_data() take a size_t
>   hw/ipmi: Assert outlen > outpos
>   chardev: Let qemu_chr_fe_write[_all] use size_t type argument
>   chardev: Let qemu_chr_write[_all] use size_t
>
>  chardev/baum.c                    |  6 +++---
>  chardev/char-fd.c                 |  6 +++---
>  chardev/char-fe.c                 |  4 ++--
>  chardev/char-io.c                 |  6 +++---
>  chardev/char-mux.c                |  3 ++-
>  chardev/char-pty.c                |  8 ++++----
>  chardev/char-socket.c             | 13 +++++++------
>  chardev/char-udp.c                |  8 ++++----
>  chardev/char-win.c                |  2 +-
>  chardev/char.c                    | 15 +++++++++------
>  chardev/msmouse.c                 |  4 ++--
>  chardev/spice.c                   |  2 +-
>  chardev/trace-events              |  2 +-
>  chardev/wctablet.c                |  9 +++++----
>  gdbstub.c                         |  8 ++++----
>  hw/bt/hci-csr.c                   |  2 +-
>  hw/char/sclpconsole-lm.c          | 12 +++++++-----
>  hw/char/spapr_vty.c               |  2 +-
>  hw/char/terminal3270.c            |  7 ++++---
>  hw/char/virtio-console.c          |  2 +-
>  hw/char/xen_console.c             | 24 +++++++++++++++---------
>  hw/ipmi/ipmi_bmc_extern.c         |  3 ++-
>  hw/tpm/tpm_emulator.c             |  7 ++++---
>  hw/usb/redirect.c                 |  6 +++++-
>  hw/virtio/vhost-user.c            | 14 ++++++++------
>  include/chardev/char-fd.h         |  2 +-
>  include/chardev/char-fe.h         |  4 ++--
>  include/chardev/char-io.h         |  2 +-
>  include/chardev/char.h            |  4 ++--
>  include/hw/ppc/spapr_vio.h        |  2 +-
>  include/hw/s390x/ebcdic.h         |  8 ++++----
>  include/hw/virtio/virtio-serial.h |  2 +-
>  include/sysemu/replay.h           |  2 +-
>  net/filter-mirror.c               |  2 +-
>  replay/replay-char.c              |  2 +-
>  stubs/replay.c                    |  2 +-
>  ui/console.c                      |  6 +++---
>  ui/gtk.c                          |  2 +-
>  38 files changed, 119 insertions(+), 96 deletions(-)
>
> --
> 2.20.1
>

On Wed, Feb 20, 2019 at 11:53:42AM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Feb 20, 2019 at 2:02 AM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
> >
> > Hi,
> >
> > This series convert the chardev::qemu_chr_write() to take unsigned
> > length argument. To do so I went through all caller and checked if
> > there are no negative value possible.
> 
> 
> Changing signedness is problematic and can easily introduce bugs that
> are easy to miss during review.
> 
> I agree with Cornelia about idiomatic use of int. Changing "int" for
> "size_t" isn't systematically a clear win.
> 
> Even Google C++ style recommends to avoid unsigned types "(except for
> representing bitfields or modular arithmetic). Do not use an unsigned
> type merely to assert that a variable is non-negative."
> https://google.github.io/styleguide/cppguide.html#Integer_Types - see rationale
> 
> Since Paolo you suggested the change, could you give some convincing
> arguments that it's worth taking the plunge?

The chardev write/read methods will end up calling libc read/write
methods, whose parameters are "size_t count".

Thus if there is QEMU code that could currently (mistakenly) pass a
negative value for length to qemu_chr_write, unless something stops
it, this is going to be cast to a size_t when we finally call read/
write on the FD, leading to a large positive value & array out of
bounds read/write. 

IOW we already have inconsistent use of signed vs unsigned in our code
which has potential to cause bugs. Converting chardev to use size_t
we get rid fo the mismatch with the underlying libc APIs we call,
which ultimately eliminates an area of risk longer term. There is a
chance it could uncover some pre-existing dormant bugs, but provided
we do due diligence to check callers I think its a win to be consistent
with libc APIs in size_t usage for read/write.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 2/20/19 5:30 AM, Daniel P. Berrangé wrote:

>> Since Paolo you suggested the change, could you give some convincing
>> arguments that it's worth taking the plunge?
> 
> The chardev write/read methods will end up calling libc read/write
> methods, whose parameters are "size_t count".

In my mind, that's the convincing reason. We should model our read/write
after the libc read/write, which means size_t input and ssize_t returns.

> 
> Thus if there is QEMU code that could currently (mistakenly) pass a
> negative value for length to qemu_chr_write, unless something stops
> it, this is going to be cast to a size_t when we finally call read/
> write on the FD, leading to a large positive value & array out of
> bounds read/write. 
> 
> IOW we already have inconsistent use of signed vs unsigned in our code
> which has potential to cause bugs. Converting chardev to use size_t
> we get rid fo the mismatch with the underlying libc APIs we call,
> which ultimately eliminates an area of risk longer term. There is a
> chance it could uncover some pre-existing dormant bugs, but provided
> we do due diligence to check callers I think its a win to be consistent
> with libc APIs in size_t usage for read/write.

And hopefully this exercise of making the conversion serves as a good
audit to help us gain confidence in our code and/or fix bugs it uncovers.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Wed, 20 Feb 2019 11:53:42 +0100
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> Hi
> 
> On Wed, Feb 20, 2019 at 2:02 AM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
> >
> > Hi,
> >
> > This series convert the chardev::qemu_chr_write() to take unsigned
> > length argument. To do so I went through all caller and checked if
> > there are no negative value possible.  
> 
> 
> Changing signedness is problematic and can easily introduce bugs that
> are easy to miss during review.
> 
> I agree with Cornelia about idiomatic use of int. Changing "int" for
> "size_t" isn't systematically a clear win.
> 
> Even Google C++ style recommends to avoid unsigned types "(except for
> representing bitfields or modular arithmetic). Do not use an unsigned
> type merely to assert that a variable is non-negative."
> https://google.github.io/styleguide/cppguide.html#Integer_Types - see rationale
> 
> Since Paolo you suggested the change, could you give some convincing
> arguments that it's worth taking the plunge?

FWIW, using an explicitly unsigned type for a length sounds fine; but
not all conversions are really convincing (albeit not wrong).