From: Eric Blake <eblake@redhat.com> To: "Daniel P. Berrangé" <berrange@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com> Cc: "Li Zhijian" <lizhijian@cn.fujitsu.com>, "Michael S. Tsirkin" <mst@redhat.com>, "Jason Wang" <jasowang@redhat.com>, "Zhang Chen" <zhangckid@gmail.com>, qemu-devel <qemu-devel@nongnu.org>, "Gerd Hoffmann" <kraxel@redhat.com>, "Stefano Stabellini" <sstabellini@kernel.org>, "Halil Pasic" <pasic@linux.ibm.com>, "Christian Borntraeger" <borntraeger@de.ibm.com>, "Anthony Perard" <anthony.perard@citrix.com>, xen-devel@lists.xenproject.org, "Philippe Mathieu-Daudé" <philmd@redhat.com>, "Corey Minyard" <minyard@acm.org>, "Amit Shah" <amit@kernel.org>, qemu-s390x@nongnu.org, "Paul Durrant" <paul.durrant@citrix.com>, "Pavel Dovgalyuk" <pavel.dovgaluk@ispras.ru>, "Samuel Thibault" <samuel.thibault@ens-lyon.org>, "David Gibson" <david@gibson.dropbear.id.au>, "Prasad J Pandit" <pjp@fedoraproject.org>, "Cornelia Huck" <cohuck@redhat.com>, qemu-ppc@nongnu.org, "Paolo Bonzini" <pbonzini@redhat.com>, "Stefan Berger" <stefanb@linux.ibm.com> Subject: Re: [Qemu-devel] [PATCH v3 00/25] chardev: Convert qemu_chr_write() to take a size_t argument Date: Wed, 20 Feb 2019 08:20:04 -0600 [thread overview] Message-ID: <2917cc10-3382-b4ce-061f-5ab8e45ea3f7@redhat.com> (raw) In-Reply-To: <20190220113016.GD21870@redhat.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Eric Blake <eblake@redhat.com> To: "Daniel P. Berrangé" <berrange@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com> Cc: "Li Zhijian" <lizhijian@cn.fujitsu.com>, "Michael S. Tsirkin" <mst@redhat.com>, "Jason Wang" <jasowang@redhat.com>, qemu-devel <qemu-devel@nongnu.org>, "Gerd Hoffmann" <kraxel@redhat.com>, "Stefano Stabellini" <sstabellini@kernel.org>, "Samuel Thibault" <samuel.thibault@ens-lyon.org>, "Halil Pasic" <pasic@linux.ibm.com>, "Christian Borntraeger" <borntraeger@de.ibm.com>, "Anthony Perard" <anthony.perard@citrix.com>, xen-devel@lists.xenproject.org, "Philippe Mathieu-Daudé" <philmd@redhat.com>, "Corey Minyard" <minyard@acm.org>, "Amit Shah" <amit@kernel.org>, qemu-s390x@nongnu.org, "Paul Durrant" <paul.durrant@citrix.com>, "Pavel Dovgalyuk" <pavel.dovgaluk@ispras.ru>, "Zhang Chen" <zhangckid@gmail.com>, "David Gibson" <david@gibson.dropbear.id.au>, "Prasad J Pandit" <pjp@fedoraproject.org>, "Cornelia Huck" <cohuck@redhat.com>, qemu-ppc@nongnu.org, "Paolo Bonzini" <pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 00/25] chardev: Convert qemu_chr_write() to take a size_t argument Date: Wed, 20 Feb 2019 08:20:04 -0600 [thread overview] Message-ID: <2917cc10-3382-b4ce-061f-5ab8e45ea3f7@redhat.com> (raw) In-Reply-To: <20190220113016.GD21870@redhat.com> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-02-20 15:24 UTC|newest] Thread overview: 132+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-02-20 1:02 [Qemu-devel] [PATCH v3 00/25] chardev: Convert qemu_chr_write() to take a size_t argument Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 01/25] chardev: Simplify IOWatchPoll::fd_can_read as a GSourceFunc Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 9:45 ` [Qemu-devel] " Marc-André Lureau 2019-02-20 9:45 ` Marc-André Lureau 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 02/25] chardev: Assert IOCanReadHandler can not be negative Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 10:03 ` [Qemu-devel] " Marc-André Lureau 2019-02-20 11:13 ` Philippe Mathieu-Daudé 2019-02-20 11:13 ` Philippe Mathieu-Daudé 2019-02-22 0:39 ` [Qemu-devel] " Philippe Mathieu-Daudé 2019-02-22 0:39 ` Philippe Mathieu-Daudé 2019-02-20 10:03 ` Marc-André Lureau 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 03/25] chardev/wctablet: Use unsigned type to hold unsigned value Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 7:32 ` [Qemu-devel] " Gerd Hoffmann 2019-02-20 7:32 ` Gerd Hoffmann 2019-02-20 10:17 ` Marc-André Lureau 2019-02-20 10:17 ` [Qemu-devel] " Marc-André Lureau 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 04/25] chardev: Let qemu_chr_be_can_write() return a size_t types Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 10:40 ` Marc-André Lureau 2019-02-20 10:40 ` [Qemu-devel] " Marc-André Lureau 2019-02-20 11:26 ` Philippe Mathieu-Daudé 2019-02-20 11:26 ` Philippe Mathieu-Daudé 2019-02-20 13:28 ` [Qemu-devel] " Marc-André Lureau 2019-02-20 13:28 ` Marc-André Lureau 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 05/25] gdbstub: Use size_t for strlen() return value Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 10:57 ` Marc-André Lureau 2019-02-20 10:57 ` [Qemu-devel] " Marc-André Lureau 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 06/25] gdbstub: Use size_t to hold GDBState::last_packet_len Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 10:59 ` [Qemu-devel] " Marc-André Lureau 2019-02-20 10:59 ` Marc-André Lureau 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 07/25] gdbstub: Let put_buffer() use size_t Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 11:02 ` [Qemu-devel] " Marc-André Lureau 2019-02-20 11:02 ` Marc-André Lureau 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 08/25] ui/gtk: Remove pointless cast Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 7:32 ` [Qemu-devel] " Gerd Hoffmann 2019-02-20 7:32 ` Gerd Hoffmann 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 09/25] vhost-user: Express sizeof with size_t Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 11:06 ` Marc-André Lureau 2019-02-20 11:06 ` [Qemu-devel] " Marc-André Lureau 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 10/25] usb-redir: Verify usbredirparser_write get called with positive count Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 7:32 ` [Qemu-devel] " Gerd Hoffmann 2019-02-20 7:32 ` Gerd Hoffmann 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 11/25] xen: Let xencons_send() take a 'size' argument Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 11:07 ` [Qemu-devel] " Marc-André Lureau 2019-02-20 11:07 ` Marc-André Lureau 2019-02-21 9:34 ` [Qemu-devel] " Paul Durrant 2019-02-21 9:34 ` Paul Durrant 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 12/25] xen: Let buffer_append() return the size consumed Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 11:13 ` [Qemu-devel] " Marc-André Lureau 2019-02-20 11:13 ` Marc-André Lureau 2019-02-20 1:02 ` [Qemu-devel] [RFC PATCH v3 13/25] xen: Let buffer_append() return a size_t Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-21 9:54 ` [Qemu-devel] " Paul Durrant 2019-02-21 9:54 ` Paul Durrant 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 14/25] virtio-serial: Let VirtIOSerialPortClass::have_data() use size_t Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 11:21 ` [Qemu-devel] " Marc-André Lureau 2019-02-20 11:21 ` Marc-André Lureau 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 15/25] spapr-vty: Let vty_putchars() " Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 1:39 ` [Qemu-devel] " David Gibson 2019-02-20 1:39 ` David Gibson 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 16/25] tpm: Use size_t to hold sizes Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 11:22 ` [Qemu-devel] " Marc-André Lureau 2019-02-20 11:22 ` Marc-André Lureau 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 17/25] net/filter-mirror: Use size_t Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 11:23 ` [Qemu-devel] " Marc-André Lureau 2019-02-20 11:23 ` Marc-André Lureau 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 18/25] s390x/3270: Let insert_IAC_escape_char() use size_t Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 9:37 ` [Qemu-devel] " Cornelia Huck 2019-02-20 9:37 ` Cornelia Huck 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 19/25] s390/ebcdic: Use size_t to iterate over arrays Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 9:40 ` [Qemu-devel] " Cornelia Huck 2019-02-20 9:40 ` Cornelia Huck 2019-02-20 11:37 ` [Qemu-devel] " Philippe Mathieu-Daudé 2019-02-20 11:37 ` Philippe Mathieu-Daudé 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 20/25] s390x/sclp: Use a const variable to improve readability Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 10:53 ` [Qemu-devel] " Cornelia Huck 2019-02-20 10:53 ` Cornelia Huck 2019-03-08 19:12 ` [Qemu-devel] " Philippe Mathieu-Daudé 2019-03-08 19:12 ` Philippe Mathieu-Daudé 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 21/25] s390x/sclp: Use size_t in process_mdb() Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 10:53 ` [Qemu-devel] " Cornelia Huck 2019-02-20 10:53 ` Cornelia Huck 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 22/25] s390x/sclp: Let write_console_data() take a size_t Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 10:54 ` [Qemu-devel] " Cornelia Huck 2019-02-20 10:54 ` Cornelia Huck 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 23/25] hw/ipmi: Assert outlen > outpos Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 13:36 ` [Qemu-devel] " Marc-André Lureau 2019-02-20 13:36 ` Marc-André Lureau 2019-02-20 13:36 ` [Qemu-devel] " Corey Minyard 2019-02-20 13:36 ` Corey Minyard 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 24/25] chardev: Let qemu_chr_fe_write[_all] use size_t type argument Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 13:44 ` [Qemu-devel] " Marc-André Lureau 2019-02-20 13:44 ` Marc-André Lureau 2019-02-20 1:02 ` [Qemu-devel] [PATCH v3 25/25] chardev: Let qemu_chr_write[_all] use size_t Philippe Mathieu-Daudé 2019-02-20 1:02 ` Philippe Mathieu-Daudé 2019-02-20 10:38 ` [Qemu-devel] " Daniel P. Berrangé 2019-02-20 10:42 ` Marc-André Lureau 2019-02-20 10:42 ` Marc-André Lureau 2019-02-20 11:31 ` Philippe Mathieu-Daudé 2019-02-20 11:31 ` Philippe Mathieu-Daudé 2019-02-20 10:38 ` Daniel P. Berrangé 2019-02-20 10:53 ` [Qemu-devel] [PATCH v3 00/25] chardev: Convert qemu_chr_write() to take a size_t argument Marc-André Lureau 2019-02-20 10:53 ` Marc-André Lureau 2019-02-20 10:57 ` [Qemu-devel] " Cornelia Huck 2019-02-20 10:57 ` Cornelia Huck 2019-02-20 11:30 ` [Qemu-devel] " Daniel P. Berrangé 2019-02-20 11:30 ` Daniel P. Berrangé 2019-02-20 14:20 ` Eric Blake [this message] 2019-02-20 14:20 ` Eric Blake
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=2917cc10-3382-b4ce-061f-5ab8e45ea3f7@redhat.com \ --to=eblake@redhat.com \ --cc=amit@kernel.org \ --cc=anthony.perard@citrix.com \ --cc=berrange@redhat.com \ --cc=borntraeger@de.ibm.com \ --cc=cohuck@redhat.com \ --cc=david@gibson.dropbear.id.au \ --cc=jasowang@redhat.com \ --cc=kraxel@redhat.com \ --cc=lizhijian@cn.fujitsu.com \ --cc=marcandre.lureau@redhat.com \ --cc=minyard@acm.org \ --cc=mst@redhat.com \ --cc=pasic@linux.ibm.com \ --cc=paul.durrant@citrix.com \ --cc=pavel.dovgaluk@ispras.ru \ --cc=pbonzini@redhat.com \ --cc=philmd@redhat.com \ --cc=pjp@fedoraproject.org \ --cc=qemu-devel@nongnu.org \ --cc=qemu-ppc@nongnu.org \ --cc=qemu-s390x@nongnu.org \ --cc=samuel.thibault@ens-lyon.org \ --cc=sstabellini@kernel.org \ --cc=stefanb@linux.ibm.com \ --cc=xen-devel@lists.xenproject.org \ --cc=zhangckid@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.