From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel =?utf-8?B?UC4gQmVycmFuZ8Op?= Subject: Re: [Qemu-devel] [PATCH v3 00/25] chardev: Convert qemu_chr_write() to take a size_t argument Date: Wed, 20 Feb 2019 11:30:16 +0000 Message-ID: <20190220113016.GD21870@redhat.com> References: <20190220010232.18731-1-philmd@redhat.com> Reply-To: Daniel =?utf-8?B?UC4gQmVycmFuZ8Op?= Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from us1-rack-dfw2.inumbo.com ([104.130.134.6]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1gwQ5s-0007of-8M for xen-devel@lists.xenproject.org; Wed, 20 Feb 2019 11:31:16 +0000 Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" To: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Cc: Li Zhijian , "Michael S. Tsirkin" , Jason Wang , Zhang Chen , qemu-devel , Gerd Hoffmann , Stefano Stabellini , Halil Pasic , Christian Borntraeger , Anthony Perard , xen-devel@lists.xenproject.org, Philippe =?utf-8?Q?Mathieu-Daud=C3=A9?= , Corey Minyard , Amit Shah , qemu-s390x@nongnu.org, Paul Durrant , Pavel Dovgalyuk , Samuel Thibault , David Gibson , Prasad J Pandit , Cornelia Huck , qemu-ppc@nongnu.org, Paolo Bonzini Stef List-Id: xen-devel@lists.xenproject.org T24gV2VkLCBGZWIgMjAsIDIwMTkgYXQgMTE6NTM6NDJBTSArMDEwMCwgTWFyYy1BbmRyw6kgTHVy ZWF1IHdyb3RlOgo+IEhpCj4gCj4gT24gV2VkLCBGZWIgMjAsIDIwMTkgYXQgMjowMiBBTSBQaGls aXBwZSBNYXRoaWV1LURhdWTDqQo+IDxwaGlsbWRAcmVkaGF0LmNvbT4gd3JvdGU6Cj4gPgo+ID4g SGksCj4gPgo+ID4gVGhpcyBzZXJpZXMgY29udmVydCB0aGUgY2hhcmRldjo6cWVtdV9jaHJfd3Jp dGUoKSB0byB0YWtlIHVuc2lnbmVkCj4gPiBsZW5ndGggYXJndW1lbnQuIFRvIGRvIHNvIEkgd2Vu dCB0aHJvdWdoIGFsbCBjYWxsZXIgYW5kIGNoZWNrZWQgaWYKPiA+IHRoZXJlIGFyZSBubyBuZWdh dGl2ZSB2YWx1ZSBwb3NzaWJsZS4KPiAKPiAKPiBDaGFuZ2luZyBzaWduZWRuZXNzIGlzIHByb2Js ZW1hdGljIGFuZCBjYW4gZWFzaWx5IGludHJvZHVjZSBidWdzIHRoYXQKPiBhcmUgZWFzeSB0byBt aXNzIGR1cmluZyByZXZpZXcuCj4gCj4gSSBhZ3JlZSB3aXRoIENvcm5lbGlhIGFib3V0IGlkaW9t YXRpYyB1c2Ugb2YgaW50LiBDaGFuZ2luZyAiaW50IiBmb3IKPiAic2l6ZV90IiBpc24ndCBzeXN0 ZW1hdGljYWxseSBhIGNsZWFyIHdpbi4KPiAKPiBFdmVuIEdvb2dsZSBDKysgc3R5bGUgcmVjb21t ZW5kcyB0byBhdm9pZCB1bnNpZ25lZCB0eXBlcyAiKGV4Y2VwdCBmb3IKPiByZXByZXNlbnRpbmcg Yml0ZmllbGRzIG9yIG1vZHVsYXIgYXJpdGhtZXRpYykuIERvIG5vdCB1c2UgYW4gdW5zaWduZWQK PiB0eXBlIG1lcmVseSB0byBhc3NlcnQgdGhhdCBhIHZhcmlhYmxlIGlzIG5vbi1uZWdhdGl2ZS4i Cj4gaHR0cHM6Ly9nb29nbGUuZ2l0aHViLmlvL3N0eWxlZ3VpZGUvY3BwZ3VpZGUuaHRtbCNJbnRl Z2VyX1R5cGVzIC0gc2VlIHJhdGlvbmFsZQo+IAo+IFNpbmNlIFBhb2xvIHlvdSBzdWdnZXN0ZWQg dGhlIGNoYW5nZSwgY291bGQgeW91IGdpdmUgc29tZSBjb252aW5jaW5nCj4gYXJndW1lbnRzIHRo YXQgaXQncyB3b3J0aCB0YWtpbmcgdGhlIHBsdW5nZT8KClRoZSBjaGFyZGV2IHdyaXRlL3JlYWQg bWV0aG9kcyB3aWxsIGVuZCB1cCBjYWxsaW5nIGxpYmMgcmVhZC93cml0ZQptZXRob2RzLCB3aG9z ZSBwYXJhbWV0ZXJzIGFyZSAic2l6ZV90IGNvdW50Ii4KClRodXMgaWYgdGhlcmUgaXMgUUVNVSBj b2RlIHRoYXQgY291bGQgY3VycmVudGx5IChtaXN0YWtlbmx5KSBwYXNzIGEKbmVnYXRpdmUgdmFs dWUgZm9yIGxlbmd0aCB0byBxZW11X2Nocl93cml0ZSwgdW5sZXNzIHNvbWV0aGluZyBzdG9wcwpp dCwgdGhpcyBpcyBnb2luZyB0byBiZSBjYXN0IHRvIGEgc2l6ZV90IHdoZW4gd2UgZmluYWxseSBj YWxsIHJlYWQvCndyaXRlIG9uIHRoZSBGRCwgbGVhZGluZyB0byBhIGxhcmdlIHBvc2l0aXZlIHZh bHVlICYgYXJyYXkgb3V0IG9mCmJvdW5kcyByZWFkL3dyaXRlLiAKCklPVyB3ZSBhbHJlYWR5IGhh dmUgaW5jb25zaXN0ZW50IHVzZSBvZiBzaWduZWQgdnMgdW5zaWduZWQgaW4gb3VyIGNvZGUKd2hp Y2ggaGFzIHBvdGVudGlhbCB0byBjYXVzZSBidWdzLiBDb252ZXJ0aW5nIGNoYXJkZXYgdG8gdXNl IHNpemVfdAp3ZSBnZXQgcmlkIGZvIHRoZSBtaXNtYXRjaCB3aXRoIHRoZSB1bmRlcmx5aW5nIGxp YmMgQVBJcyB3ZSBjYWxsLAp3aGljaCB1bHRpbWF0ZWx5IGVsaW1pbmF0ZXMgYW4gYXJlYSBvZiBy aXNrIGxvbmdlciB0ZXJtLiBUaGVyZSBpcyBhCmNoYW5jZSBpdCBjb3VsZCB1bmNvdmVyIHNvbWUg cHJlLWV4aXN0aW5nIGRvcm1hbnQgYnVncywgYnV0IHByb3ZpZGVkCndlIGRvIGR1ZSBkaWxpZ2Vu Y2UgdG8gY2hlY2sgY2FsbGVycyBJIHRoaW5rIGl0cyBhIHdpbiB0byBiZSBjb25zaXN0ZW50Cndp dGggbGliYyBBUElzIGluIHNpemVfdCB1c2FnZSBmb3IgcmVhZC93cml0ZS4KClJlZ2FyZHMsCkRh bmllbAotLSAKfDogaHR0cHM6Ly9iZXJyYW5nZS5jb20gICAgICAtby0gICAgaHR0cHM6Ly93d3cu ZmxpY2tyLmNvbS9waG90b3MvZGJlcnJhbmdlIDp8Cnw6IGh0dHBzOi8vbGlidmlydC5vcmcgICAg ICAgICAtby0gICAgICAgICAgICBodHRwczovL2ZzdG9wMTM4LmJlcnJhbmdlLmNvbSA6fAp8OiBo dHRwczovL2VudGFuZ2xlLXBob3RvLm9yZyAgICAtby0gICAgaHR0cHM6Ly93d3cuaW5zdGFncmFt LmNvbS9kYmVycmFuZ2UgOnwKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fClhlbi1kZXZlbCBtYWlsaW5nIGxpc3QKWGVuLWRldmVsQGxpc3RzLnhlbnByb2pl Y3Qub3JnCmh0dHBzOi8vbGlzdHMueGVucHJvamVjdC5vcmcvbWFpbG1hbi9saXN0aW5mby94ZW4t ZGV2ZWw= From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:60365) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gwQ5i-0005iQ-Qq for qemu-devel@nongnu.org; Wed, 20 Feb 2019 06:31:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gwQ5h-0003nF-KC for qemu-devel@nongnu.org; Wed, 20 Feb 2019 06:31:06 -0500 Date: Wed, 20 Feb 2019 11:30:16 +0000 From: Daniel =?utf-8?B?UC4gQmVycmFuZ8Op?= Message-ID: <20190220113016.GD21870@redhat.com> Reply-To: Daniel =?utf-8?B?UC4gQmVycmFuZ8Op?= References: <20190220010232.18731-1-philmd@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 00/25] chardev: Convert qemu_chr_write() to take a size_t argument List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Cc: Philippe =?utf-8?Q?Mathieu-Daud=C3=A9?= , Li Zhijian , "Michael S. Tsirkin" , Jason Wang , qemu-devel , Gerd Hoffmann , Stefano Stabellini , Samuel Thibault , Halil Pasic , Christian Borntraeger , Anthony Perard , xen-devel@lists.xenproject.org, Corey Minyard , Amit Shah , qemu-s390x@nongnu.org, Paul Durrant , Pavel Dovgalyuk , Zhang Chen , David Gibson , Prasad J Pandit , Cornelia Huck , qemu-ppc@nongnu.org, Paolo Bonzini , Stefan Berger On Wed, Feb 20, 2019 at 11:53:42AM +0100, Marc-Andr=C3=A9 Lureau wrote: > Hi >=20 > On Wed, Feb 20, 2019 at 2:02 AM Philippe Mathieu-Daud=C3=A9 > 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. >=20 >=20 > Changing signedness is problematic and can easily introduce bugs that > are easy to miss during review. >=20 > I agree with Cornelia about idiomatic use of int. Changing "int" for > "size_t" isn't systematically a clear win. >=20 > 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 r= ationale >=20 > 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.=20 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 --=20 |: https://berrange.com -o- https://www.flickr.com/photos/dberran= ge :| |: https://libvirt.org -o- https://fstop138.berrange.c= om :| |: https://entangle-photo.org -o- https://www.instagram.com/dberran= ge :|