netdev - Re: [RTNL]: Validate hardware and broadcast address attribute for RTM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 22 Feb 2008 14:05:34 +0100
From:	Patrick McHardy <kaber@...sh.net>
To:	Thomas Graf <tgraf@...g.ch>
CC:	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [RTNL]: Validate hardware and broadcast address attribute for
 RTM_NEWLINK

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@...g.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().
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html