qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Daniel P . Berrangé
Subject: Re: [PATCH] net: tap: check if the file descriptor is valid before using it
Date: Tue, 30 Jun 2020 10:31:48 +0100
User-agent: Mutt/1.14.3 (2020-06-14)

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

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]