[PATCH v1] RDMA/nldev: Add length check for IFLA_BOND_ARP_IP_TARGET parsing

[PATCH v1] RDMA/nldev: Add length check for IFLA_BOND_ARP_IP_TARGET parsing

[Lin Ma]

The nla_for_each_nested parsing in function
nldev_stat_set_counter_dynamic_doit() 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 4 byte integer.

This patch adds the check based on nla_len() just as other code does,
see how bond_changelink (drivers/net/bonding/bond_netlink.c) parses
IFLA_BOND_NS_IP6_TARGET.

Fixes: 3c3c1f141639 ("RDMA/nldev: Allow optional-counter status configuration through RDMA netlink")
Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
 drivers/infiniband/core/nldev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
index d5d3e4f0de77..74635c23b371 100644
--- a/drivers/infiniband/core/nldev.c
+++ b/drivers/infiniband/core/nldev.c
@@ -1989,6 +1989,11 @@ static int nldev_stat_set_counter_dynamic_doit(struct nlattr *tb[],
 
 	nla_for_each_nested(entry_attr, tb[RDMA_NLDEV_ATTR_STAT_HWCOUNTERS],
 			    rem) {
+		if (nla_len(entry_attr) < sizeof(index)) {
+			ret = -EINVAL;
+			goto out;
+		}
+
 		index = nla_get_u32(entry_attr);
 		if ((index >= stats->num_counters) ||
 		    !(stats->descs[index].flags & IB_STAT_FLAG_OPTIONAL)) {
-- 
2.17.1

Re: [PATCH v1] RDMA/nldev: Add length check for IFLA_BOND_ARP_IP_TARGET parsing

[Leon Romanovsky]

On Sun, Jul 23, 2023 at 03:45:04PM +0800, Lin Ma wrote:
> The nla_for_each_nested parsing in function
> nldev_stat_set_counter_dynamic_doit() 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 4 byte integer.

1. Subject of this patch doesn't really match the change.
2. See my comment on your i40e patch.
https://lore.kernel.org/netdev/20230724174435.GA11388@unreal/

Thanks

Re: [PATCH v1] RDMA/nldev: Add length check for IFLA_BOND_ARP_IP_TARGET parsing

[Lin Ma]

Hello Leon,

> 
> On Sun, Jul 23, 2023 at 03:45:04PM +0800, Lin Ma wrote:
> > The nla_for_each_nested parsing in function
> > nldev_stat_set_counter_dynamic_doit() 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 4 byte integer.
> 
> 1. Subject of this patch doesn't really match the change.

My bad, a stupid mistake. I will fix that and prepare another patch.

> 2. See my comment on your i40e patch.
> https://lore.kernel.org/netdev/20230724174435.GA11388@unreal/
> 

Yeah I have seen that. Just as Jakub said, empty netlink attributes are valid 
(they are viewed as flag). The point is that different attribute has different
length requirement. For this specific code, the RDMA_NLDEV_ATTR_STAT_HWCOUNTERS
attribute is a nested one whose inner attributes should be NLA_U32. But as you
can see in variable nldev_policy, the description does not use nested policy to
enfore that, which results in the bug discussed in my commit message.

 [RDMA_NLDEV_ATTR_STAT_HWCOUNTERS]       = { .type = NLA_NESTED },

The elegant fix could be add the nested policy description to nldev_policy while
this is toublesome as no existing nla_attr has been given to this nested nlattr.
Hence, add the length check is the simplest solution and you can see such nla_len
check code all over the kernel.

> Thanks

Regards
Lin

Re: [PATCH v1] RDMA/nldev: Add length check for IFLA_BOND_ARP_IP_TARGET parsing

[Leon Romanovsky]

On Tue, Jul 25, 2023 at 08:11:58AM +0800, Lin Ma wrote:
> Hello Leon,
> 
> > 
> > On Sun, Jul 23, 2023 at 03:45:04PM +0800, Lin Ma wrote:
> > > The nla_for_each_nested parsing in function
> > > nldev_stat_set_counter_dynamic_doit() 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 4 byte integer.
> > 
> > 1. Subject of this patch doesn't really match the change.
> 
> My bad, a stupid mistake. I will fix that and prepare another patch.
> 
> > 2. See my comment on your i40e patch.
> > https://lore.kernel.org/netdev/20230724174435.GA11388@unreal/
> > 
> 
> Yeah I have seen that. Just as Jakub said, empty netlink attributes are valid 
> (they are viewed as flag). The point is that different attribute has different
> length requirement. For this specific code, the RDMA_NLDEV_ATTR_STAT_HWCOUNTERS
> attribute is a nested one whose inner attributes should be NLA_U32. But as you
> can see in variable nldev_policy, the description does not use nested policy to
> enfore that, which results in the bug discussed in my commit message.
> 
>  [RDMA_NLDEV_ATTR_STAT_HWCOUNTERS]       = { .type = NLA_NESTED },
> 
> The elegant fix could be add the nested policy description to nldev_policy while
> this is toublesome as no existing nla_attr has been given to this nested nlattr.
> Hence, add the length check is the simplest solution and you can see such nla_len
> check code all over the kernel.

Right, and this is what bothers me.

I would more than happy to change nla_for_each_nested() to be something
like nla_for_each_nested_type(...., sizeof(u32)), which will skip empty
lines, for code which can't have them.

Thanks

> 
> > Thanks
> 
> Regards
> Lin

Re: [PATCH v1] RDMA/nldev: Add length check for IFLA_BOND_ARP_IP_TARGET parsing

[Lin Ma]

Hi Leon,

> 
> Right, and this is what bothers me.
> 
> I would more than happy to change nla_for_each_nested() to be something
> like nla_for_each_nested_type(...., sizeof(u32)), which will skip empty
> lines, for code which can't have them.
> 
> Thanks
> 

Well, nla_for_each_nested_type(...., sizeof(u32)) seems good if the nested
attribute is sure to contain only one type of attribute with constant length.
(like the case of RDMA_NLDEV_ATTR_STAT_HWCOUNTERS).

I accept that it is another elegant solution here. But efforts are needed to
make sure if this is true for other cases.

Regards
Lin

Re: [PATCH v1] RDMA/nldev: Add length check for IFLA_BOND_ARP_IP_TARGET parsing

[Jakub Kicinski]

On Tue, 25 Jul 2023 08:25:57 +0300 Leon Romanovsky wrote:
> > Yeah I have seen that. Just as Jakub said, empty netlink attributes are valid 
> > (they are viewed as flag). The point is that different attribute has different
> > length requirement. For this specific code, the RDMA_NLDEV_ATTR_STAT_HWCOUNTERS
> > attribute is a nested one whose inner attributes should be NLA_U32. But as you
> > can see in variable nldev_policy, the description does not use nested policy to
> > enfore that, which results in the bug discussed in my commit message.
> > 
> >  [RDMA_NLDEV_ATTR_STAT_HWCOUNTERS]       = { .type = NLA_NESTED },
> > 
> > The elegant fix could be add the nested policy description to nldev_policy while
> > this is toublesome as no existing nla_attr has been given to this nested nlattr.
> > Hence, add the length check is the simplest solution and you can see such nla_len
> > check code all over the kernel.  
> 
> Right, and this is what bothers me.
> 
> I would more than happy to change nla_for_each_nested() to be something
> like nla_for_each_nested_type(...., sizeof(u32)), which will skip empty
> lines, for code which can't have them.

In general the idea of auto-skipping stuff kernel doesn't recognize
is a bit old school. Better direction would be extending the policy
validation to cover use cases for such loops.

Re: [PATCH v1] RDMA/nldev: Add length check for IFLA_BOND_ARP_IP_TARGET parsing

[Leon Romanovsky]

On Tue, Jul 25, 2023 at 10:14:05AM -0700, Jakub Kicinski wrote:
> On Tue, 25 Jul 2023 08:25:57 +0300 Leon Romanovsky wrote:
> > > Yeah I have seen that. Just as Jakub said, empty netlink attributes are valid 
> > > (they are viewed as flag). The point is that different attribute has different
> > > length requirement. For this specific code, the RDMA_NLDEV_ATTR_STAT_HWCOUNTERS
> > > attribute is a nested one whose inner attributes should be NLA_U32. But as you
> > > can see in variable nldev_policy, the description does not use nested policy to
> > > enfore that, which results in the bug discussed in my commit message.
> > > 
> > >  [RDMA_NLDEV_ATTR_STAT_HWCOUNTERS]       = { .type = NLA_NESTED },
> > > 
> > > The elegant fix could be add the nested policy description to nldev_policy while
> > > this is toublesome as no existing nla_attr has been given to this nested nlattr.
> > > Hence, add the length check is the simplest solution and you can see such nla_len
> > > check code all over the kernel.  
> > 
> > Right, and this is what bothers me.
> > 
> > I would more than happy to change nla_for_each_nested() to be something
> > like nla_for_each_nested_type(...., sizeof(u32)), which will skip empty
> > lines, for code which can't have them.
> 
> In general the idea of auto-skipping stuff kernel doesn't recognize
> is a bit old school. Better direction would be extending the policy
> validation to cover use cases for such loops.

I'm all in for any solution which will help for average developer to write
netlink code without mistakes.

Thanks

Re: [PATCH v1] RDMA/nldev: Add length check for IFLA_BOND_ARP_IP_TARGET parsing

[Lin Ma]

Hello there,

> > > > Yeah I have seen that. Just as Jakub said, empty netlink attributes are valid 
> > > > (they are viewed as flag). The point is that different attribute has different
> > > > length requirement. For this specific code, the RDMA_NLDEV_ATTR_STAT_HWCOUNTERS
> > > > attribute is a nested one whose inner attributes should be NLA_U32. But as you
> > > > can see in variable nldev_policy, the description does not use nested policy to
> > > > enfore that, which results in the bug discussed in my commit message.
> > > > 
> > > >  [RDMA_NLDEV_ATTR_STAT_HWCOUNTERS]       = { .type = NLA_NESTED },
> > > > 
> > > > The elegant fix could be add the nested policy description to nldev_policy while
> > > > this is toublesome as no existing nla_attr has been given to this nested nlattr.
> > > > Hence, add the length check is the simplest solution and you can see such nla_len
> > > > check code all over the kernel.  
> > > 
> > > Right, and this is what bothers me.
> > > 
> > > I would more than happy to change nla_for_each_nested() to be something
> > > like nla_for_each_nested_type(...., sizeof(u32)), which will skip empty
> > > lines, for code which can't have them.
> > 
> > In general the idea of auto-skipping stuff kernel doesn't recognize
> > is a bit old school. Better direction would be extending the policy
> > validation to cover use cases for such loops.
> 
> I'm all in for any solution which will help for average developer to write
> netlink code without mistakes.
> 
> Thanks

I have just come out a new solution for such length issues. Please see
* https://lore.kernel.org/all/20230731121247.3972783-1-linma@zju.edu.cn/T/#u
* https://lore.kernel.org/all/20230731121324.3973136-1-linma@zju.edu.cn/T/#u

I'm not sure adding additional validation logic in the main nlattr code is
the best solution. Still, after investigating the code, the len field can
be very suitable for handling the NLA_NESTED cases here. And the developer
can do manual parsing with better nla_policy-based checking too.

If this idea is applied, I will also write a script to clean up other
nla_len patches based on the nla_policy check.

Regards
Lin

Re: [PATCH v1] RDMA/nldev: Add length check for IFLA_BOND_ARP_IP_TARGET parsing

[Leon Romanovsky]

On Mon, Jul 31, 2023 at 08:33:02PM +0800, Lin Ma wrote:
> Hello there,
> 
> > > > > Yeah I have seen that. Just as Jakub said, empty netlink attributes are valid 
> > > > > (they are viewed as flag). The point is that different attribute has different
> > > > > length requirement. For this specific code, the RDMA_NLDEV_ATTR_STAT_HWCOUNTERS
> > > > > attribute is a nested one whose inner attributes should be NLA_U32. But as you
> > > > > can see in variable nldev_policy, the description does not use nested policy to
> > > > > enfore that, which results in the bug discussed in my commit message.
> > > > > 
> > > > >  [RDMA_NLDEV_ATTR_STAT_HWCOUNTERS]       = { .type = NLA_NESTED },
> > > > > 
> > > > > The elegant fix could be add the nested policy description to nldev_policy while
> > > > > this is toublesome as no existing nla_attr has been given to this nested nlattr.
> > > > > Hence, add the length check is the simplest solution and you can see such nla_len
> > > > > check code all over the kernel.  
> > > > 
> > > > Right, and this is what bothers me.
> > > > 
> > > > I would more than happy to change nla_for_each_nested() to be something
> > > > like nla_for_each_nested_type(...., sizeof(u32)), which will skip empty
> > > > lines, for code which can't have them.
> > > 
> > > In general the idea of auto-skipping stuff kernel doesn't recognize
> > > is a bit old school. Better direction would be extending the policy
> > > validation to cover use cases for such loops.
> > 
> > I'm all in for any solution which will help for average developer to write
> > netlink code without mistakes.
> > 
> > Thanks
> 
> I have just come out a new solution for such length issues. Please see
> * https://lore.kernel.org/all/20230731121247.3972783-1-linma@zju.edu.cn/T/#u
> * https://lore.kernel.org/all/20230731121324.3973136-1-linma@zju.edu.cn/T/#u
> 
> I'm not sure adding additional validation logic in the main nlattr code is
> the best solution. Still, after investigating the code, the len field can
> be very suitable for handling the NLA_NESTED cases here. And the developer
> can do manual parsing with better nla_policy-based checking too.
> 
> If this idea is applied, I will also write a script to clean up other
> nla_len patches based on the nla_policy check.

It looks like Jakub didn't like the idea and we will need to add your
sizeof checks all other the place.

Thanks

> 
> Regards
> Lin