diff mbox series

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

Message ID 20220331123502.6472-1-florent.fourcot@wifirst.fr (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] rtnetlink: return ENODEV when ifname does not exist and group is given | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2 this patch: 2
netdev/cc_maintainers fail 5 maintainers not CCed: davem@davemloft.net petrm@nvidia.com pabeni@redhat.com idosch@nvidia.com kuba@kernel.org
netdev/build_clang success Errors and warnings before: 9 this patch: 9
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 2 this patch: 2
netdev/checkpatch warning WARNING: line length of 96 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Florent Fourcot March 31, 2022, 12:35 p.m. UTC 
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(-)

Comments

Jakub Kicinski April 1, 2022, 3:36 a.m. UTC | #1 
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]))
Florent Fourcot April 1, 2022, 1:57 p.m. UTC | #2 
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,
Jakub Kicinski April 1, 2022, 5:25 p.m. UTC | #3 
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...)
diff mbox series

Patch

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;
 	}