[PATCH net-next] rtnetlink: return ENODEV when ifname does not exist and group is given

[PATCH net-next] rtnetlink: return ENODEV when ifname does not exist and group is given

[Florent Fourcot]

When the interface does not exist, and a group is given, the given
parameters are being set to all interfaces of the given group. The given
IFNAME/ALT_IF_NAME are being ignored in that case.

That can be dangerous since a typo (or a deleted interface) can produce
weird side effects for caller:

Case 1:

 IFLA_IFNAME=valid_interface
 IFLA_GROUP=1
 MTU=1234

Case 1 will update MTU and group of the given interface "valid_interface".

Case 2:

 IFLA_IFNAME=doesnotexist
 IFLA_GROUP=1
 MTU=1234

Case 2 will update MTU of all interfaces in group 1. IFLA_IFNAME is
ignored in this case

This behaviour is not consistent and dangerous. In order to fix this issue,
we now return ENODEV when the given IFNAME does not exist.

Signed-off-by: Florent Fourcot <florent.fourcot@wifirst.fr>
Signed-off-by: Brian Baboch <brian.baboch@wifirst.fr>
---
 net/core/rtnetlink.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 159c9c61e6af..3313419bbcba 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3417,10 +3417,15 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	}
 
 	if (!(nlh->nlmsg_flags & NLM_F_CREATE)) {
-		if (ifm->ifi_index == 0 && tb[IFLA_GROUP])
+		if (ifm->ifi_index == 0 && tb[IFLA_GROUP]) {
+			if (tb[IFLA_IFNAME] || tb[IFLA_ALT_IFNAME]) {
+				NL_SET_ERR_MSG(extack, "Cannot find interface with given name");
+				return -ENODEV;
+			}
 			return rtnl_group_changelink(skb, net,
 						nla_get_u32(tb[IFLA_GROUP]),
 						ifm, extack, tb);
+		}
 		return -ENODEV;
 	}
 
-- 
2.30.2

Re: [PATCH net-next] rtnetlink: return ENODEV when ifname does not exist and group is given

[Jakub Kicinski]

On Thu, 31 Mar 2022 14:35:02 +0200 Florent Fourcot wrote:
> From: Florent Fourcot <florent.fourcot@wifirst.fr>
> To: netdev@vger.kernel.org
> Cc: Florent Fourcot <florent.fourcot@wifirst.fr>, Brian Baboch <brian.baboch@wifirst.fr>

Please run the patches thru ./scripts/get_maintainer.pl.
netdev is high volume (and Gmail hates it), we really need people 
to CC potential reviewers.

> When the interface does not exist, and a group is given, the given
> parameters are being set to all interfaces of the given group. The given
> IFNAME/ALT_IF_NAME are being ignored in that case.
> 
> That can be dangerous since a typo (or a deleted interface) can produce
> weird side effects for caller:
> 
> Case 1:
> 
>  IFLA_IFNAME=valid_interface
>  IFLA_GROUP=1
>  MTU=1234
> 
> Case 1 will update MTU and group of the given interface "valid_interface".
> 
> Case 2:
> 
>  IFLA_IFNAME=doesnotexist
>  IFLA_GROUP=1
>  MTU=1234
> 
> Case 2 will update MTU of all interfaces in group 1. IFLA_IFNAME is
> ignored in this case
> 
> This behaviour is not consistent and dangerous. In order to fix this issue,
> we now return ENODEV when the given IFNAME does not exist.

> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 159c9c61e6af..3313419bbcba 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3417,10 +3417,15 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	}
>  
>  	if (!(nlh->nlmsg_flags & NLM_F_CREATE)) {
> -		if (ifm->ifi_index == 0 && tb[IFLA_GROUP])
> +		if (ifm->ifi_index == 0 && tb[IFLA_GROUP]) {
> +			if (tb[IFLA_IFNAME] || tb[IFLA_ALT_IFNAME]) {
> +				NL_SET_ERR_MSG(extack, "Cannot find interface with given name");
> +				return -ENODEV;
> +			}
>  			return rtnl_group_changelink(skb, net,
>  						nla_get_u32(tb[IFLA_GROUP]),
>  						ifm, extack, tb);
> +		}
>  		return -ENODEV;
>  	}

Would it be slightly cleaner to have a similar check in
validate_linkmsg()? Something like:

	if (!dev && !ifm->ifi_index && tb[IFLA_GROUP] &&
	    (tb[IFLA_IFNAME] || tb[IFLA_ALT_IFNAME]))

Re: [PATCH net-next] rtnetlink: return ENODEV when ifname does not exist and group is given

[Florent Fourcot]

Hello,

> Please run the patches thru ./scripts/get_maintainer.pl.
> netdev is high volume (and Gmail hates it), we really need people 
> to CC potential reviewers.

Sorry for that. I was unable to choose relevant reviewers in 
get_maintainer output (too few or too many lines). I will add CC in next 
post.

> 
> Would it be slightly cleaner to have a similar check in
> validate_linkmsg()? Something like:
> 
> 	if (!dev && !ifm->ifi_index && tb[IFLA_GROUP] &&
> 	    (tb[IFLA_IFNAME] || tb[IFLA_ALT_IFNAME]))


That will add some complexity in validate_linkmsg, since we will need to 
check NLM_F_CREATE flag before (it's probably not an error when 
NLM_F_CREATE is set), and to give ifm as argument to check ifi_index.

I do not have a strong opinion on this topic, and if you think your idea 
is a better solution I can give a try in a v2.

Thanks,

-- 
Florent Fourcot

Re: [PATCH net-next] rtnetlink: return ENODEV when ifname does not exist and group is given

[Jakub Kicinski]

On Fri, 1 Apr 2022 15:57:52 +0200 Florent Fourcot wrote:
> > Would it be slightly cleaner to have a similar check in
> > validate_linkmsg()? Something like:
> > 
> > 	if (!dev && !ifm->ifi_index && tb[IFLA_GROUP] &&
> > 	    (tb[IFLA_IFNAME] || tb[IFLA_ALT_IFNAME]))  
> 
> 
> That will add some complexity in validate_linkmsg, since we will need to 
> check NLM_F_CREATE flag before (it's probably not an error when 
> NLM_F_CREATE is set), and to give ifm as argument to check ifi_index.
> 
> I do not have a strong opinion on this topic, and if you think your idea 
> is a better solution I can give a try in a v2.

Okay, fair. Please repost on Monday, tho, net-next is currently closed
(contrary to some status pages may be telling you...)