Re: [Qemu-devel] [PATCH v3 00/25] chardev: Convert qemu_chr_write() to take a size_t argument

From: "Daniel P. Berrangé" <berrange@redhat.com>
To: "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>
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	[thread overview]
Message-ID: <20190220113016.GD21870@redhat.com> (raw)
In-Reply-To: <CAMxuvazRLabuNiGdKb9HSxyqJ+9NhxJkqkv824BTFj95j5u9rA@mail.gmail.com>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel