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 <[email protected]>
---
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;
+
mode = nla_get_u16(attr);
if ((mode != BRIDGE_MODE_VEPA) &&
(mode != BRIDGE_MODE_VEB))
--
2.17.1
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 <[email protected]>
> ---
> 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/)
--
pw-bot: cr
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