[PATCH v2] openvswitch: Fix condition check in do_execute_actions() by using nla_ok()

[PATCH v2] openvswitch: Fix condition check in do_execute_actions() by using nla_ok()

[Jiasheng Jiang]

Just using 'rem > 0' might be unsafe, so it's better
to use the nla_ok() instead.
Because we can see from the nla_next() that
'*remaining' might be smaller than 'totlen'. And nla_ok()
will avoid it happening.
For example, ovs_dp_process_packet() -> ovs_execute_actions()
-> do_execute_actions(), and attr comes from OVS_CB(skb)->input_vport,
which restores the received packet from the user space.

Fixes: ccb1352e76cff0524e7ccb2074826a092dd13016
('net: Add Open vSwitch kernel components.')
Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
---
 net/openvswitch/actions.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 77d924a..c23537f 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -1238,8 +1238,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 	const struct nlattr *a;
 	int rem;
 
-	for (a = attr, rem = len; rem > 0;
-	     a = nla_next(a, &rem)) {
+	nla_for_each_attr(a, attr, len, rem) {
 		int err = 0;
 
 		switch (nla_type(a)) {
-- 
2.7.4

Re: [PATCH v2] openvswitch: Fix condition check in do_execute_actions() by using nla_ok()

[Jakub Kicinski]

On Fri, 17 Sep 2021 08:07:14 +0000 Jiasheng Jiang wrote:
> Just using 'rem > 0' might be unsafe, so it's better
> to use the nla_ok() instead.
> Because we can see from the nla_next() that
> '*remaining' might be smaller than 'totlen'. And nla_ok()
> will avoid it happening.
> For example, ovs_dp_process_packet() -> ovs_execute_actions()
> -> do_execute_actions(), and attr comes from OVS_CB(skb)->input_vport,  
> which restores the received packet from the user space.

Right but that's the call trace for where the actions are executed.
Not where they are constructed.

As far as I can tell the action list is constructed in the kernel 
by __ovs_nla_copy_actions(). Since kernel does the formatting, it
can trust the contents are correct. We normally require nla_ok()
when handling input directly from user space, which is not the 
case in do_execute_actions().

And since kernel is sure that the input is correct the extra checking
just adds to the datapath overhead.

Unless you can point out how exactly the input could be invalid 
at this point I'd suggest we leave this code as is. Perhaps add
a comment explaining why input is trusted.

Thanks!

> Fixes: ccb1352e76cff0524e7ccb2074826a092dd13016
> ('net: Add Open vSwitch kernel components.')

FWIW the correct format would have been:

Fixes: ccb1352e76cf ("net: Add Open vSwitch kernel components.")

> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>