| Message ID | 20130110113108.GF13451@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Jan 10, 2013 at 01:31:08PM +0200, Michael S. Tsirkin wrote: > At the moment, we check owner when we enable queue in tun. > This seems redundant and will break some valid uses > where fd is passed around: I think TUNSETOWNER is there > to prevent others from attaching to a persistent device not > owned by them. Here the fd is already attached, > enabling/disabling queue is more like read/write. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > Note: this is unrelated to Stefan's bugfix. This should go into 3.8-rc. Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote: > At the moment, we check owner when we enable queue in tun. > This seems redundant and will break some valid uses > where fd is passed around: I think TUNSETOWNER is there > to prevent others from attaching to a persistent device not > owned by them. Here the fd is already attached, > enabling/disabling queue is more like read/write. It also change the number of queues of the tuntap, maybe we should limit this. Note that if management layer does not call TUNSETOWNER, the check is just a nop. So if management layer want to limit the behavior, it's its duty to do this correctly. > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > Note: this is unrelated to Stefan's bugfix. > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index fbd106e..78e3225 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr) > tun = tfile->detached; > if (!tun) > ret = -EINVAL; > - else if (tun_not_capable(tun)) > - ret = -EPERM; > else > ret = tun_attach(tun, file); > } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) { > tun = rcu_dereference_protected(tfile->tun, > lockdep_rtnl_is_held()); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote: > On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote: > > At the moment, we check owner when we enable queue in tun. > > This seems redundant and will break some valid uses > > where fd is passed around: I think TUNSETOWNER is there > > to prevent others from attaching to a persistent device not > > owned by them. Here the fd is already attached, > > enabling/disabling queue is more like read/write. > > It also change the number of queues of the tuntap, maybe we should limit > this. Number of active queues? Why does it matter? Max number of queues is already limited by SETIFF. > Note that if management layer does not call TUNSETOWNER, the check > is just a nop. So if management layer want to limit the behavior, it's > its duty to do this correctly. The point is that management limits tun to allow SETIFF from libvirt only, then passes the fds to qemu. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > --- > > > > Note: this is unrelated to Stefan's bugfix. > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > index fbd106e..78e3225 100644 > > --- a/drivers/net/tun.c > > +++ b/drivers/net/tun.c > > @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr) > > tun = tfile->detached; > > if (!tun) > > ret = -EINVAL; > > - else if (tun_not_capable(tun)) > > - ret = -EPERM; > > else > > ret = tun_attach(tun, file); > > } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) { > > tun = rcu_dereference_protected(tfile->tun, > > lockdep_rtnl_is_held()); > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/10/2013 10:19 PM, Michael S. Tsirkin wrote: > On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote: >> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote: >>> At the moment, we check owner when we enable queue in tun. >>> This seems redundant and will break some valid uses >>> where fd is passed around: I think TUNSETOWNER is there >>> to prevent others from attaching to a persistent device not >>> owned by them. Here the fd is already attached, >>> enabling/disabling queue is more like read/write. >> It also change the number of queues of the tuntap, maybe we should limit >> this. > Number of active queues? Why does it matter? > Max number of queues is already limited by SETIFF. Yes the number of active(real) queues in the kernel net device and this changing may introduce other events such uevent. With this patch, even if a owner is set for tap, every user could change the number of real queues which I don't think is not expected. Without this patch, we can limit a user that just do read and write. > >> Note that if management layer does not call TUNSETOWNER, the check >> is just a nop. So if management layer want to limit the behavior, it's >> its duty to do this correctly. > The point is that management limits tun to allow SETIFF from libvirt > only, then passes the fds to qemu. Yes, but looks like libvirt does not call TUNSETOWNER before passing it to qemu, so we're ok even without this patch. And if libvirt want to do this, it can just call TUNSETOWNER to the user of qemu. >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> >>> --- >>> >>> Note: this is unrelated to Stefan's bugfix. >>> >>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c >>> index fbd106e..78e3225 100644 >>> --- a/drivers/net/tun.c >>> +++ b/drivers/net/tun.c >>> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr) >>> tun = tfile->detached; >>> if (!tun) >>> ret = -EINVAL; >>> - else if (tun_not_capable(tun)) >>> - ret = -EPERM; >>> else >>> ret = tun_attach(tun, file); >>> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) { >>> tun = rcu_dereference_protected(tfile->tun, >>> lockdep_rtnl_is_held()); > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 10, 2013 at 10:27:20PM +0800, Jason Wang wrote: > On 01/10/2013 10:19 PM, Michael S. Tsirkin wrote: > > On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote: > >> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote: > >>> At the moment, we check owner when we enable queue in tun. > >>> This seems redundant and will break some valid uses > >>> where fd is passed around: I think TUNSETOWNER is there > >>> to prevent others from attaching to a persistent device not > >>> owned by them. Here the fd is already attached, > >>> enabling/disabling queue is more like read/write. > >> It also change the number of queues of the tuntap, maybe we should limit > >> this. > > Number of active queues? Why does it matter? > > Max number of queues is already limited by SETIFF. > > Yes the number of active(real) queues in the kernel net device and this > changing may introduce other events such uevent. How can it trigger a uevent? > With this patch, even > if a owner is set for tap, every user could change the number of real > queues which I don't think is not expected. Without this patch, we can > limit a user that just do read and write. In the end if you want very fine tuned security policy you have to use an LSM. Here we are talking about the expected usage without an LSM. There, enabling/disabling queues is just an optimization: if an application wants to process data from a single thread it's better off getting it through a single fd. Having to channel threading changes through a priveledged proxy would be very awkward. > > > >> Note that if management layer does not call TUNSETOWNER, the check > >> is just a nop. So if management layer want to limit the behavior, it's > >> its duty to do this correctly. > > The point is that management limits tun to allow SETIFF from libvirt > > only, then passes the fds to qemu. > > Yes, but looks like libvirt does not call TUNSETOWNER before passing it > to qemu, so we're ok even without this patch. And if libvirt want to do > this, it can just call TUNSETOWNER to the user of qemu. No, that would allow qemu to do SETIFF which we don't want. > >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >>> > >>> --- > >>> > >>> Note: this is unrelated to Stefan's bugfix. > >>> > >>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c > >>> index fbd106e..78e3225 100644 > >>> --- a/drivers/net/tun.c > >>> +++ b/drivers/net/tun.c > >>> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr) > >>> tun = tfile->detached; > >>> if (!tun) > >>> ret = -EINVAL; > >>> - else if (tun_not_capable(tun)) > >>> - ret = -EPERM; > >>> else > >>> ret = tun_attach(tun, file); > >>> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) { > >>> tun = rcu_dereference_protected(tfile->tun, > >>> lockdep_rtnl_is_held()); > > -- > > To unsubscribe from this list: send the line "unsubscribe netdev" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/10/2013 10:41 PM, Michael S. Tsirkin wrote: > On Thu, Jan 10, 2013 at 10:27:20PM +0800, Jason Wang wrote: >> On 01/10/2013 10:19 PM, Michael S. Tsirkin wrote: >>> On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote: >>>> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote: >>>>> At the moment, we check owner when we enable queue in tun. >>>>> This seems redundant and will break some valid uses >>>>> where fd is passed around: I think TUNSETOWNER is there >>>>> to prevent others from attaching to a persistent device not >>>>> owned by them. Here the fd is already attached, >>>>> enabling/disabling queue is more like read/write. >>>> It also change the number of queues of the tuntap, maybe we should limit >>>> this. >>> Number of active queues? Why does it matter? >>> Max number of queues is already limited by SETIFF. >> Yes the number of active(real) queues in the kernel net device and this >> changing may introduce other events such uevent. > How can it trigger a uevent? netif_set_real_num_{tx|rx}_queues() will update the queue kobjects which may trigger an uevent. > >> With this patch, even >> if a owner is set for tap, every user could change the number of real >> queues which I don't think is not expected. Without this patch, we can >> limit a user that just do read and write. > In the end if you want very fine tuned security policy you have to > use an LSM. > > Here we are talking about the expected usage without an LSM. > There, enabling/disabling queues is just an optimization: > if an application wants to process data from a single thread > it's better off getting it through a single fd. > Having to channel threading changes through a priveledged > proxy would be very awkward. Yes, but we have something similar like bridge-helper in qemu which create devices through a privileged proxy. >>>> Note that if management layer does not call TUNSETOWNER, the check >>>> is just a nop. So if management layer want to limit the behavior, it's >>>> its duty to do this correctly. >>> The point is that management limits tun to allow SETIFF from libvirt >>> only, then passes the fds to qemu. >> Yes, but looks like libvirt does not call TUNSETOWNER before passing it >> to qemu, so we're ok even without this patch. And if libvirt want to do >> this, it can just call TUNSETOWNER to the user of qemu. > No, that would allow qemu to do SETIFF which we don't want. True, I was wrong. > >>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>>> >>>>> --- >>>>> >>>>> Note: this is unrelated to Stefan's bugfix. >>>>> >>>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c >>>>> index fbd106e..78e3225 100644 >>>>> --- a/drivers/net/tun.c >>>>> +++ b/drivers/net/tun.c >>>>> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr) >>>>> tun = tfile->detached; >>>>> if (!tun) >>>>> ret = -EINVAL; >>>>> - else if (tun_not_capable(tun)) >>>>> - ret = -EPERM; >>>>> else >>>>> ret = tun_attach(tun, file); >>>>> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) { >>>>> tun = rcu_dereference_protected(tfile->tun, >>>>> lockdep_rtnl_is_held()); >>> -- >>> To unsubscribe from this list: send the line "unsubscribe netdev" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 10, 2013 at 10:47:49PM +0800, Jason Wang wrote: > On 01/10/2013 10:41 PM, Michael S. Tsirkin wrote: > > On Thu, Jan 10, 2013 at 10:27:20PM +0800, Jason Wang wrote: > >> On 01/10/2013 10:19 PM, Michael S. Tsirkin wrote: > >>> On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote: > >>>> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote: > >>>>> At the moment, we check owner when we enable queue in tun. > >>>>> This seems redundant and will break some valid uses > >>>>> where fd is passed around: I think TUNSETOWNER is there > >>>>> to prevent others from attaching to a persistent device not > >>>>> owned by them. Here the fd is already attached, > >>>>> enabling/disabling queue is more like read/write. > >>>> It also change the number of queues of the tuntap, maybe we should limit > >>>> this. > >>> Number of active queues? Why does it matter? > >>> Max number of queues is already limited by SETIFF. > >> Yes the number of active(real) queues in the kernel net device and this > >> changing may introduce other events such uevent. > > How can it trigger a uevent? > > netif_set_real_num_{tx|rx}_queues() will update the queue kobjects which > may trigger an uevent. Look SETOWNER is a tool intended mostly for persistent taps, where you give a specific user the rights to attach to specific taps but not others. The uevent issue is preventing a DOS by a uevent flood? Then it applies to persistent and non persistent as one. So if one cares about this one should use an LSM or we can add a separate capability to limit this if we care enough. > > > >> With this patch, even > >> if a owner is set for tap, every user could change the number of real > >> queues which I don't think is not expected. Without this patch, we can > >> limit a user that just do read and write. > > In the end if you want very fine tuned security policy you have to > > use an LSM. > > > > Here we are talking about the expected usage without an LSM. > > There, enabling/disabling queues is just an optimization: > > if an application wants to process data from a single thread > > it's better off getting it through a single fd. > > Having to channel threading changes through a priveledged > > proxy would be very awkward. > > Yes, but we have something similar like bridge-helper in qemu which > create devices through a privileged proxy. This only happens on startup. Threading changes can happen at any time. > >>>> Note that if management layer does not call TUNSETOWNER, the check > >>>> is just a nop. So if management layer want to limit the behavior, it's > >>>> its duty to do this correctly. > >>> The point is that management limits tun to allow SETIFF from libvirt > >>> only, then passes the fds to qemu. > >> Yes, but looks like libvirt does not call TUNSETOWNER before passing it > >> to qemu, so we're ok even without this patch. And if libvirt want to do > >> this, it can just call TUNSETOWNER to the user of qemu. > > No, that would allow qemu to do SETIFF which we don't want. > > True, I was wrong. > > > >>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >>>>> > >>>>> --- > >>>>> > >>>>> Note: this is unrelated to Stefan's bugfix. > >>>>> > >>>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c > >>>>> index fbd106e..78e3225 100644 > >>>>> --- a/drivers/net/tun.c > >>>>> +++ b/drivers/net/tun.c > >>>>> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr) > >>>>> tun = tfile->detached; > >>>>> if (!tun) > >>>>> ret = -EINVAL; > >>>>> - else if (tun_not_capable(tun)) > >>>>> - ret = -EPERM; > >>>>> else > >>>>> ret = tun_attach(tun, file); > >>>>> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) { > >>>>> tun = rcu_dereference_protected(tfile->tun, > >>>>> lockdep_rtnl_is_held()); > >>> -- > >>> To unsubscribe from this list: send the line "unsubscribe netdev" in > >>> the body of a message to majordomo@vger.kernel.org > >>> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/10/2013 11:10 PM, Michael S. Tsirkin wrote: > On Thu, Jan 10, 2013 at 10:47:49PM +0800, Jason Wang wrote: >> On 01/10/2013 10:41 PM, Michael S. Tsirkin wrote: >>> On Thu, Jan 10, 2013 at 10:27:20PM +0800, Jason Wang wrote: >>>> On 01/10/2013 10:19 PM, Michael S. Tsirkin wrote: >>>>> On Thu, Jan 10, 2013 at 10:08:03PM +0800, Jason Wang wrote: >>>>>> On 01/10/2013 07:31 PM, Michael S. Tsirkin wrote: >>>>>>> At the moment, we check owner when we enable queue in tun. >>>>>>> This seems redundant and will break some valid uses >>>>>>> where fd is passed around: I think TUNSETOWNER is there >>>>>>> to prevent others from attaching to a persistent device not >>>>>>> owned by them. Here the fd is already attached, >>>>>>> enabling/disabling queue is more like read/write. >>>>>> It also change the number of queues of the tuntap, maybe we should limit >>>>>> this. >>>>> Number of active queues? Why does it matter? >>>>> Max number of queues is already limited by SETIFF. >>>> Yes the number of active(real) queues in the kernel net device and this >>>> changing may introduce other events such uevent. >>> How can it trigger a uevent? >> netif_set_real_num_{tx|rx}_queues() will update the queue kobjects which >> may trigger an uevent. > Look SETOWNER is a tool intended mostly for persistent taps, > where you give a specific user the rights to attach to > specific taps but not others. True. > The uevent issue is preventing a DOS by a uevent flood? > Then it applies to persistent and non persistent as one. > So if one cares about this one should use an LSM > or we can add a separate capability to limit this if we > care enough. Ok. >>>> With this patch, even >>>> if a owner is set for tap, every user could change the number of real >>>> queues which I don't think is not expected. Without this patch, we can >>>> limit a user that just do read and write. >>> In the end if you want very fine tuned security policy you have to >>> use an LSM. >>> >>> Here we are talking about the expected usage without an LSM. >>> There, enabling/disabling queues is just an optimization: >>> if an application wants to process data from a single thread >>> it's better off getting it through a single fd. >>> Having to channel threading changes through a priveledged >>> proxy would be very awkward. >> Yes, but we have something similar like bridge-helper in qemu which >> create devices through a privileged proxy. > This only happens on startup. Threading changes can happen > at any time. Yes. So no objection from my side. Thanks for the explanation. >>>>>> Note that if management layer does not call TUNSETOWNER, the check >>>>>> is just a nop. So if management layer want to limit the behavior, it's >>>>>> its duty to do this correctly. >>>>> The point is that management limits tun to allow SETIFF from libvirt >>>>> only, then passes the fds to qemu. >>>> Yes, but looks like libvirt does not call TUNSETOWNER before passing it >>>> to qemu, so we're ok even without this patch. And if libvirt want to do >>>> this, it can just call TUNSETOWNER to the user of qemu. >>> No, that would allow qemu to do SETIFF which we don't want. >> True, I was wrong. >>>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>>>>> >>>>>>> --- >>>>>>> >>>>>>> Note: this is unrelated to Stefan's bugfix. >>>>>>> >>>>>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c >>>>>>> index fbd106e..78e3225 100644 >>>>>>> --- a/drivers/net/tun.c >>>>>>> +++ b/drivers/net/tun.c >>>>>>> @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr) >>>>>>> tun = tfile->detached; >>>>>>> if (!tun) >>>>>>> ret = -EINVAL; >>>>>>> - else if (tun_not_capable(tun)) >>>>>>> - ret = -EPERM; >>>>>>> else >>>>>>> ret = tun_attach(tun, file); >>>>>>> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) { >>>>>>> tun = rcu_dereference_protected(tfile->tun, >>>>>>> lockdep_rtnl_is_held()); >>>>> -- >>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in >>>>> the body of a message to majordomo@vger.kernel.org >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Stefan Hajnoczi <stefanha@redhat.com> Date: Thu, 10 Jan 2013 12:55:03 +0100 > On Thu, Jan 10, 2013 at 01:31:08PM +0200, Michael S. Tsirkin wrote: >> At the moment, we check owner when we enable queue in tun. >> This seems redundant and will break some valid uses >> where fd is passed around: I think TUNSETOWNER is there >> to prevent others from attaching to a persistent device not >> owned by them. Here the fd is already attached, >> enabling/disabling queue is more like read/write. >> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> >> --- >> >> Note: this is unrelated to Stefan's bugfix. > > This should go into 3.8-rc. > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index fbd106e..78e3225 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1789,10 +1792,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr) tun = tfile->detached; if (!tun) ret = -EINVAL; - else if (tun_not_capable(tun)) - ret = -EPERM; else ret = tun_attach(tun, file); } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) { tun = rcu_dereference_protected(tfile->tun, lockdep_rtnl_is_held());
At the moment, we check owner when we enable queue in tun. This seems redundant and will break some valid uses where fd is passed around: I think TUNSETOWNER is there to prevent others from attaching to a persistent device not owned by them. Here the fd is already attached, enabling/disabling queue is more like read/write. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- Note: this is unrelated to Stefan's bugfix.