| Message ID | 20230723075042.3709043-1-linma@zju.edu.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1] i40e: Add length check for IFLA_AF_SPEC parsing | expand |
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
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))
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(+)