[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 :|
- [PATCH] net: tap: check if the file descriptor is valid before using it, Laurent Vivier, 2020/06/24
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it, Philippe Mathieu-Daudé, 2020/06/25
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it, Daniel P . Berrangé, 2020/06/25
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it, Laurent Vivier, 2020/06/25
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it, Jason Wang, 2020/06/28
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it, Laurent Vivier, 2020/06/29
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it, Jason Wang, 2020/06/30
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it, Daniel P . Berrangé, 2020/06/30
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it,
Daniel P . Berrangé <=
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it, Laurent Vivier, 2020/06/30
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it, Jason Wang, 2020/06/30
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it, Laurent Vivier, 2020/06/30
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it, Jason Wang, 2020/06/30
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it, Daniel P . Berrangé, 2020/06/30
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it, Laurent Vivier, 2020/06/30
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it, Daniel P . Berrangé, 2020/06/30
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it, Laurent Vivier, 2020/06/30
- Re: [PATCH] net: tap: check if the file descriptor is valid before using it, Jason Wang, 2020/06/30