[RTNL]: Validate hardware and broadcast address attribute for RTM_NEWLINK

[RTNL]: Validate hardware and broadcast address attribute for RTM_NEWLINK

[Thomas Graf]

RTM_NEWLINK allows for already existing links to be modified. For this
purpose do_setlink() is called which expects address attributes with a
payload length of at least dev->addr_len. This patch adds the necessary
validation for the RTM_NEWLINK case.

The address length for links to be created is not checked for now as the
actual attribute length is used when copying the address to the netdevice
structure. It might make sense to report an error if less than addr_len
bytes are provided but enforcing this might break drivers trying to be
smart with not transmitting all zero addresses.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Index: net-2.6.26/net/core/rtnetlink.c
===================================================================
--- net-2.6.26.orig/net/core/rtnetlink.c	2008-02-22 01:50:53.000000000 +0100
+++ net-2.6.26/net/core/rtnetlink.c	2008-02-22 11:28:59.000000000 +0100
@@ -726,6 +726,21 @@
 	return net;
 }
 
+static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[])
+{
+	if (dev) {
+		if (tb[IFLA_ADDRESS] &&
+		    nla_len(tb[IFLA_ADDRESS]) < dev->addr_len)
+			return -EINVAL;
+
+		if (tb[IFLA_BROADCAST] &&
+		    nla_len(tb[IFLA_BROADCAST]) < dev->addr_len)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
 		      struct nlattr **tb, char *ifname, int modified)
 {
@@ -910,12 +925,7 @@
 		goto errout;
 	}
 
-	if (tb[IFLA_ADDRESS] &&
-	    nla_len(tb[IFLA_ADDRESS]) < dev->addr_len)
-		goto errout_dev;
-
-	if (tb[IFLA_BROADCAST] &&
-	    nla_len(tb[IFLA_BROADCAST]) < dev->addr_len)
+	if ((err = validate_linkmsg(dev, tb)) < 0)
 		goto errout_dev;
 
 	err = do_setlink(dev, ifm, tb, ifname, 0);
@@ -1036,6 +1046,9 @@
 	else
 		dev = NULL;
 
+	if ((err = validate_linkmsg(dev, tb)) < 0)
+		return err;
+
 	if (tb[IFLA_LINKINFO]) {
 		err = nla_parse_nested(linkinfo, IFLA_INFO_MAX,
 				       tb[IFLA_LINKINFO], ifla_info_policy);

Re: [RTNL]: Validate hardware and broadcast address attribute for RTM_NEWLINK

[Patrick McHardy]

Thomas Graf wrote:
> RTM_NEWLINK allows for already existing links to be modified. For this
> purpose do_setlink() is called which expects address attributes with a
> payload length of at least dev->addr_len. This patch adds the necessary
> validation for the RTM_NEWLINK case.
> 
> The address length for links to be created is not checked for now as the
> actual attribute length is used when copying the address to the netdevice
> structure. It might make sense to report an error if less than addr_len
> bytes are provided but enforcing this might break drivers trying to be
> smart with not transmitting all zero addresses.
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> 
> Index: net-2.6.26/net/core/rtnetlink.c
> ===================================================================
> --- net-2.6.26.orig/net/core/rtnetlink.c	2008-02-22 01:50:53.000000000 +0100
> +++ net-2.6.26/net/core/rtnetlink.c	2008-02-22 11:28:59.000000000 +0100
> @@ -726,6 +726,21 @@
>  	return net;
>  }
>  
> +static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[])
> +{
> +	if (dev) {
> +		if (tb[IFLA_ADDRESS] &&
> +		    nla_len(tb[IFLA_ADDRESS]) < dev->addr_len)
> +			return -EINVAL;
> +
> +		if (tb[IFLA_BROADCAST] &&
> +		    nla_len(tb[IFLA_BROADCAST]) < dev->addr_len)
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
>  		      struct nlattr **tb, char *ifname, int modified)
>  {
> @@ -910,12 +925,7 @@
>  		goto errout;
>  	}
>  
> -	if (tb[IFLA_ADDRESS] &&
> -	    nla_len(tb[IFLA_ADDRESS]) < dev->addr_len)
> -		goto errout_dev;
> -
> -	if (tb[IFLA_BROADCAST] &&
> -	    nla_len(tb[IFLA_BROADCAST]) < dev->addr_len)
> +	if ((err = validate_linkmsg(dev, tb)) < 0)
>  		goto errout_dev;
>  
>  	err = do_setlink(dev, ifm, tb, ifname, 0);
> @@ -1036,6 +1046,9 @@
>  	else
>  		dev = NULL;
>  
> +	if ((err = validate_linkmsg(dev, tb)) < 0)
> +		return err;
> +

Minor nitpick: it would be more logical to put this in the

if (dev) {
  ...

branch a bit below since thats the only path that leads to
do_setlink(). That would also allow to remove the
if (dev) check from validate_linkmsg().

Re: [RTNL]: Validate hardware and broadcast address attribute for RTM_NEWLINK

[Thomas Graf]

* Patrick McHardy <kaber@trash.net> 2008-02-22 14:05
> Minor nitpick: it would be more logical to put this in the
> 
> if (dev) {
>  ...
> 
> branch a bit below since thats the only path that leads to
> do_setlink(). That would also allow to remove the
> if (dev) check from validate_linkmsg().

I knew this question would come up :-)

The reason I did it this way is to keep validate_linkmsg() generic
and make it possible to put validation code which must also apply
to new links (dev==NULL) into that function.

Re: [RTNL]: Validate hardware and broadcast address attribute for RTM_NEWLINK

[Patrick McHardy]

Thomas Graf wrote:
> * Patrick McHardy <kaber@trash.net> 2008-02-22 14:05
>> Minor nitpick: it would be more logical to put this in the
>>
>> if (dev) {
>>  ...
>>
>> branch a bit below since thats the only path that leads to
>> do_setlink(). That would also allow to remove the
>> if (dev) check from validate_linkmsg().
> 
> I knew this question would come up :-)

:)

> The reason I did it this way is to keep validate_linkmsg() generic
> and make it possible to put validation code which must also apply
> to new links (dev==NULL) into that function.

OK, thanks for the explanation.

Re: [RTNL]: Validate hardware and broadcast address attribute for RTM_NEWLINK

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Fri, 22 Feb 2008 14:33:26 +0100

> Thomas Graf wrote:
> > The reason I did it this way is to keep validate_linkmsg() generic
> > and make it possible to put validation code which must also apply
> > to new links (dev==NULL) into that function.
> 
> OK, thanks for the explanation.

Patch applied, thanks.