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 :|