Currently tun checks the group permission even if the user have matched.
Besides going against the usual permission semantic, this has a
very interesting implication: if the tun group is not among the
supplementary groups of the tun user, then effectively no one can
access the tun device. CAP_SYS_ADMIN still can, but its the same as
not setting the tun ownership.

This patch relaxes the group checking so that either the user match
or the group match is enough. This avoids the situation when no one
can access the device even though the ownership is properly set.

Also I simplified the logic by removing the redundant inversions:
tun_not_capable() --> !tun_capable()

Signed-off-by: Stas Sergeev <stsp2@yandex.ru>

CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
CC: Jason Wang <jasowang@redhat.com>
CC: Andrew Lunn <andrew+netdev@lunn.ch>
CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Jakub Kicinski <kuba@kernel.org>
CC: Paolo Abeni <pabeni@redhat.com>
CC: netdev@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 drivers/net/tun.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 9a0f6eb32016..d35b6a48d138 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -574,14 +574,18 @@ static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb,
 	return ret;
 }
 
-static inline bool tun_not_capable(struct tun_struct *tun)
+static inline bool tun_capable(struct tun_struct *tun)
 {
 	const struct cred *cred = current_cred();
 	struct net *net = dev_net(tun->dev);
 
-	return ((uid_valid(tun->owner) && !uid_eq(cred->euid, tun->owner)) ||
-		  (gid_valid(tun->group) && !in_egroup_p(tun->group))) &&
-		!ns_capable(net->user_ns, CAP_NET_ADMIN);
+	if (ns_capable(net->user_ns, CAP_NET_ADMIN))
+		return 1;
+	if (uid_valid(tun->owner) && uid_eq(cred->euid, tun->owner))
+		return 1;
+	if (gid_valid(tun->group) && in_egroup_p(tun->group))
+		return 1;
+	return 0;
 }
 
 static void tun_set_real_num_queues(struct tun_struct *tun)
@@ -2778,7 +2782,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 		    !!(tun->flags & IFF_MULTI_QUEUE))
 			return -EINVAL;
 
-		if (tun_not_capable(tun))
+		if (!tun_capable(tun))
 			return -EPERM;
 		err = security_tun_dev_open(tun->security);
 		if (err < 0)
-- 
2.47.0

On Thu,  5 Dec 2024 10:36:14 +0300 Stas Sergeev wrote:
> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
> 
> CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>

Please avoid empty lines in the future.
I personally put a --- line between SOB and the CCs.
That way git am discards the CCs when patch is applied.

08.12.2024 04:44, Jakub Kicinski пишет:
> On Thu,  5 Dec 2024 10:36:14 +0300 Stas Sergeev wrote:
>> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
>>
>> CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Please avoid empty lines in the future.

Ok.

> I personally put a --- line between SOB and the CCs.
> That way git am discards the CCs when patch is applied.
>
I simply used the output of
get_maintainer.pl and copy/pasted
it directly to commit msg.
After doing so, git format-patch
would put --- after CCs.
How to do that properly?

On Sun, 8 Dec 2024 09:53:40 +0300 stsp wrote:
> > I personally put a --- line between SOB and the CCs.
> > That way git am discards the CCs when patch is applied.
> >  
> I simply used the output of
> get_maintainer.pl and copy/pasted
> it directly to commit msg.
> After doing so, git format-patch
> would put --- after CCs.
> How to do that properly?

You can have multiple --- markers, you can insert your marker and let
git format-patch add another.

10.12.2024 00:44, Jakub Kicinski пишет:
> On Sun, 8 Dec 2024 09:53:40 +0300 stsp wrote:
>>> I personally put a --- line between SOB and the CCs.
>>> That way git am discards the CCs when patch is applied.
>>>   
>> I simply used the output of
>> get_maintainer.pl and copy/pasted
>> it directly to commit msg.
>> After doing so, git format-patch
>> would put --- after CCs.
>> How to do that properly?
> You can have multiple --- markers, you can insert your marker and let
> git format-patch add another.

Thank you.

On Sat, 7 Dec 2024 17:44:00 -0800 Jakub Kicinski wrote:
> On Thu,  5 Dec 2024 10:36:14 +0300 Stas Sergeev wrote:
> > Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
> > 
> > CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>  
> 
> Please avoid empty lines in the future.

I mean empty lines between tags, to be clear

Stas Sergeev wrote:
> Currently tun checks the group permission even if the user have matched.
> Besides going against the usual permission semantic, this has a
> very interesting implication: if the tun group is not among the
> supplementary groups of the tun user, then effectively no one can
> access the tun device. CAP_SYS_ADMIN still can, but its the same as
> not setting the tun ownership.
> 
> This patch relaxes the group checking so that either the user match
> or the group match is enough. This avoids the situation when no one
> can access the device even though the ownership is properly set.
> 
> Also I simplified the logic by removing the redundant inversions:
> tun_not_capable() --> !tun_capable()
> 
> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
> 
> CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> CC: Jason Wang <jasowang@redhat.com>
> CC: Andrew Lunn <andrew+netdev@lunn.ch>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Eric Dumazet <edumazet@google.com>
> CC: Jakub Kicinski <kuba@kernel.org>
> CC: Paolo Abeni <pabeni@redhat.com>
> CC: netdev@vger.kernel.org
> CC: linux-kernel@vger.kernel.org

Reviewed-by: Willem de Bruijn <willemb@google.com>

A lot more readable this way too.

On Fri, Dec 6, 2024 at 12:50 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Stas Sergeev wrote:
> > Currently tun checks the group permission even if the user have matched.
> > Besides going against the usual permission semantic, this has a
> > very interesting implication: if the tun group is not among the
> > supplementary groups of the tun user, then effectively no one can
> > access the tun device. CAP_SYS_ADMIN still can, but its the same as
> > not setting the tun ownership.
> >
> > This patch relaxes the group checking so that either the user match
> > or the group match is enough. This avoids the situation when no one
> > can access the device even though the ownership is properly set.
> >
> > Also I simplified the logic by removing the redundant inversions:
> > tun_not_capable() --> !tun_capable()
> >
> > Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
> >
> > CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > CC: Jason Wang <jasowang@redhat.com>
> > CC: Andrew Lunn <andrew+netdev@lunn.ch>
> > CC: "David S. Miller" <davem@davemloft.net>
> > CC: Eric Dumazet <edumazet@google.com>
> > CC: Jakub Kicinski <kuba@kernel.org>
> > CC: Paolo Abeni <pabeni@redhat.com>
> > CC: netdev@vger.kernel.org
> > CC: linux-kernel@vger.kernel.org
>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
>
> A lot more readable this way too.
>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks