Re: [PATCH] net: tap: check if the file descriptor is valid before using it

[Daniel P . Berrangé]

On Tue, Jun 30, 2020 at 12:35:46PM +0200, Laurent Vivier wrote:
> On 30/06/2020 12:03, Jason Wang wrote:
> > 
> > On 2020/6/30 下午5:45, Laurent Vivier wrote:
> >> On 30/06/2020 11:31, Daniel P. Berrangé wrote:
> >>> On Tue, Jun 30, 2020 at 10:23:18AM +0100, Daniel P. Berrangé wrote:
> >>>> On Tue, Jun 30, 2020 at 05:21:49PM +0800, Jason Wang wrote:
> >>>>> On 2020/6/30 上午3:30, Laurent Vivier wrote:
> >>>>>> On 28/06/2020 08:31, Jason Wang wrote:
> >>>>>>> On 2020/6/25 下午7:56, Laurent Vivier wrote:
> >>>>>>>> On 25/06/2020 10:48, Daniel P. Berrangé wrote:
> >>>>>>>>> On Wed, Jun 24, 2020 at 09:00:09PM +0200, Laurent Vivier wrote:
> >>>>>>>>>> qemu_set_nonblock() checks that the file descriptor can be
> >>>>>>>>>> used and, if
> >>>>>>>>>> not, crashes QEMU. An assert() is used for that. The use of
> >>>>>>>>>> assert() is
> >>>>>>>>>> used to detect programming error and the coredump will allow
> >>>>>>>>>> to debug
> >>>>>>>>>> the problem.
> >>>>>>>>>>
> >>>>>>>>>> But in the case of the tap device, this assert() can be
> >>>>>>>>>> triggered by
> >>>>>>>>>> a misconfiguration by the user. At startup, it's not a real
> >>>>>>>>>> problem,
> >>>>>>>>>> but it
> >>>>>>>>>> can also happen during the hot-plug of a new device, and here
> >>>>>>>>>> it's a
> >>>>>>>>>> problem because we can crash a perfectly healthy system.
> >>>>>>>>> If the user/mgmt app is not correctly passing FDs, then there's
> >>>>>>>>> a whole
> >>>>>>>>> pile of bad stuff that can happen. Checking whether the FD is
> >>>>>>>>> valid is
> >>>>>>>>> only going to catch a small subset. eg consider if fd=9 refers
> >>>>>>>>> to the
> >>>>>>>>> FD that is associated with the root disk QEMU has open. We'll
> >>>>>>>>> fail to
> >>>>>>>>> setup the TAP device and close this FD, breaking the healthy
> >>>>>>>>> system
> >>>>>>>>> again.
> >>>>>>>>>
> >>>>>>>>> I'm not saying we can't check if the FD is valid, but lets be
> >>>>>>>>> clear that
> >>>>>>>>> this is not offering very much protection against a broken mgmt
> >>>>>>>>> apps
> >>>>>>>>> passing bad FDs.
> >>>>>>>>>
> >>>>>>>> I agree with you, but my only goal here is to avoid the crash in
> >>>>>>>> this
> >>>>>>>> particular case.
> >>>>>>>>
> >>>>>>>> The punishment should fit the crime.
> >>>>>>>>
> >>>>>>>> The user can think the netdev_del doesn't close the fd, and he
> >>>>>>>> can try
> >>>>>>>> to reuse it. Sending back an error is better than crashing his
> >>>>>>>> system.
> >>>>>>>> After that, if the system crashes, it will be for the good
> >>>>>>>> reasons, not
> >>>>>>>> because of an assert.
> >>>>>>> Yes. And on top of this we may try to validate the TAP via st_dev
> >>>>>>> through fstat[1].
> >>>>>> I agree, but the problem I have is to know which major(st_dev) we can
> >>>>>> allow to use.
> >>>>>>
> >>>>>> Do we allow only macvtap major number?
> >>>>>
> >>>>> Macvtap and tuntap.
> >>>>>
> >>>>>
> >>>>>> How to know the macvtap major number at user level?
> >>>>>> [it is allocated dynamically: do we need to parse /proc/devices?]
> >>>>>
> >>>>> I think we can get them through fstat for /dev/net/tun and
> >>>>> /dev/macvtapX.
> >>>> Don't assume QEMU has any permission to access to these device nodes,
> >>>> only the pre-opened FDs it is given by libvirt.
> >>> Actually permissions are the least of the problem - the device nodes
> >>> won't even exist, because QEMU's almost certainly running in a private
> >>> mount namespace with a minimal /dev populated
> >>>
> >> I'm working on a solution using /proc/devices.
> > 
> > 
> > Similar issue with /dev. There's no guarantee that qemu can access
> > /proc/devices or it may not exist (CONFIG_PROCFS).
> 
> There is a lot of things that will not work without /proc (several tools
> rely on /proc, like ps, top, lsof, mount, ...). Some information are
> only available from /proc, and if /proc is there, I think /proc/devices
> is always readable by everyone. Moreover /proc is already used by qemu
> in several places.
> 
> It can also a best effort check.
> 
> The problem with fstat() on /dev files is to guess the /dev/macvtapX as
> X varies (the same with /dev/tapY)..
> 
> > 
> >> macvtap has its own major number, but tuntap use "misc" (10) major
> >> number.
> 
> Another question: it is possible to use the "fd=" parameter with macvtap
> as macvtap creates a /dev/tapY device, but how to do that with tuntap
> that does not create a /dev/tapY device?


I think we should step back and ask why we need to check this at all.

IMHO, if the passed-in FD works with the syscalls that tap-linux.c
is executing, then that shows the FD is suitable for QEMU. The problem
is that many of the tap APIs don't use "Error **errp" parameters to
report errors, so we can't catch the failures. IOW, instead of checking
the FD major/minor number, we should make the existing code be better
at reporting errors, so they can be fed back to the QMP console
gracefully.


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