[v1] i40e: Add length check for IFLA_AF_SPEC parsing

  The nla_for_each_nested parsing in function i40e_ndo_bridge_setlink()
does not check the length of the attribute. This can lead to an
out-of-attribute read and allow a malformed nlattr (e.g., length 0) to
be viewed as a 2 byte integer.

This patch adds the check based on nla_len() just as other code does,
see how bnxt_bridge_setlink (drivers/net/ethernet/broadcom/bnxt/bnxt.c)
parses IFLA_AF_SPEC: type checking plus length checking.

Fixes: 51616018dd1b ("i40e: Add support for getlink, setlink ndo ops")
Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 3 +++
 1 file changed, 3 insertions(+)
  

On Sun, Jul 23, 2023 at 03:50:42PM +0800, Lin Ma wrote:
> The nla_for_each_nested parsing in function i40e_ndo_bridge_setlink()
> does not check the length of the attribute. This can lead to an
> out-of-attribute read and allow a malformed nlattr (e.g., length 0) to
> be viewed as a 2 byte integer.
> 
> This patch adds the check based on nla_len() just as other code does,
> see how bnxt_bridge_setlink (drivers/net/ethernet/broadcom/bnxt/bnxt.c)
> parses IFLA_AF_SPEC: type checking plus length checking.
> 
> Fixes: 51616018dd1b ("i40e: Add support for getlink, setlink ndo ops")
> Signed-off-by: Lin Ma <linma@zju.edu.cn>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 29ad1797adce..6363357bdeeb 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -13186,6 +13186,9 @@ static int i40e_ndo_bridge_setlink(struct net_device *dev,
>  		if (nla_type(attr) != IFLA_BRIDGE_MODE)
>  			continue;
>  
> +		if (nla_len(attr) < sizeof(mode))
> +			return -EINVAL;
> +

I see that you added this hunk to all users of nla_for_each_nested(), it
will be great to make that iterator to skip such empty attributes.

However, i don't know nettlink good enough to say if your change is
valid in first place.

Thanks

>  		mode = nla_get_u16(attr);
>  		if ((mode != BRIDGE_MODE_VEPA) &&
>  		    (mode != BRIDGE_MODE_VEB))
> -- 
> 2.17.1
> 
>
  

On Mon, 24 Jul 2023 20:44:35 +0300 Leon Romanovsky wrote:
> > @@ -13186,6 +13186,9 @@ static int i40e_ndo_bridge_setlink(struct net_device *dev,
> >  		if (nla_type(attr) != IFLA_BRIDGE_MODE)
> >  			continue;
> >  
> > +		if (nla_len(attr) < sizeof(mode))
> > +			return -EINVAL;
> > +  
> 
> I see that you added this hunk to all users of nla_for_each_nested(), it
> will be great to make that iterator to skip such empty attributes.
> 
> However, i don't know nettlink good enough to say if your change is
> valid in first place.

Empty attributes are valid, we can't do that.

But there's a loop in rtnl_bridge_setlink() which checks the attributes.
We can add the check there instead of all users, as Leon points out.
(Please just double check that all ndo_bridge_setlink implementation
expect this value to be a u16, they should/)
  

Hello Jakub,

> On Mon, 24 Jul 2023 20:44:35 +0300 Leon Romanovsky wrote:
> > > @@ -13186,6 +13186,9 @@ static int i40e_ndo_bridge_setlink(struct net_device *dev,
> > >  		if (nla_type(attr) != IFLA_BRIDGE_MODE)
> > >  			continue;
> > >  
> > > +		if (nla_len(attr) < sizeof(mode))
> > > +			return -EINVAL;
> > > +  
> > 
> > I see that you added this hunk to all users of nla_for_each_nested(), it
> > will be great to make that iterator to skip such empty attributes.
> > 
> > However, i don't know nettlink good enough to say if your change is
> > valid in first place.
> 
> Empty attributes are valid, we can't do that.
> 
> But there's a loop in rtnl_bridge_setlink() which checks the attributes.

Cool, I will check this out and prepare another patch.

> We can add the check there instead of all users, as Leon points out.
> (Please just double check that all ndo_bridge_setlink implementation
> expect this value to be a u16, they should/)

Okay, I will finish that ASAP

> -- 
> pw-bot: cr

Regards
Lin
  

On Mon, Jul 24, 2023 at 02:21:55PM -0700, Jakub Kicinski wrote:
> On Mon, 24 Jul 2023 20:44:35 +0300 Leon Romanovsky wrote:
> > > @@ -13186,6 +13186,9 @@ static int i40e_ndo_bridge_setlink(struct net_device *dev,
> > >  		if (nla_type(attr) != IFLA_BRIDGE_MODE)
> > >  			continue;
> > >  
> > > +		if (nla_len(attr) < sizeof(mode))
> > > +			return -EINVAL;
> > > +  
> > 
> > I see that you added this hunk to all users of nla_for_each_nested(), it
> > will be great to make that iterator to skip such empty attributes.
> > 
> > However, i don't know nettlink good enough to say if your change is
> > valid in first place.
> 
> Empty attributes are valid, we can't do that.

Maybe Lin can add special version of nla_for_each_nested() which will
skip these empty NLAs, for code which don't allow empty attributes.

> 
> But there's a loop in rtnl_bridge_setlink() which checks the attributes.
> We can add the check there instead of all users, as Leon points out.
> (Please just double check that all ndo_bridge_setlink implementation
> expect this value to be a u16, they should/)
> -- 
> pw-bot: cr
  

On Tue, 25 Jul 2023 08:40:46 +0300 Leon Romanovsky wrote:
> > Empty attributes are valid, we can't do that.  
> 
> Maybe Lin can add special version of nla_for_each_nested() which will
> skip these empty NLAs, for code which don't allow empty attributes.

It's way too arbitrary. Empty attrs are 100% legit, they are called
NLA_FLAG in policy parlance. They are basically a boolean.
  

On Tue, Jul 25, 2023 at 09:53:27AM -0700, Jakub Kicinski wrote:
> On Tue, 25 Jul 2023 08:40:46 +0300 Leon Romanovsky wrote:
> > > Empty attributes are valid, we can't do that.  
> > 
> > Maybe Lin can add special version of nla_for_each_nested() which will
> > skip these empty NLAs, for code which don't allow empty attributes.
> 
> It's way too arbitrary. Empty attrs are 100% legit, they are called
> NLA_FLAG in policy parlance. They are basically a boolean.

I afraid that these nla_length() checks will be copied all other the
kernel without any understanding and netlink API doesn't really provide
any hint when length checks are needed and when they don't.

Thank