LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] openvswitch: Fix condition check by using nla_ok()
@ 2021-09-16  1:43 Jiasheng Jiang
  2021-09-16 14:36 ` Jakub Kicinski
  0 siblings, 1 reply; 4+ messages in thread
From: Jiasheng Jiang @ 2021-09-16  1:43 UTC (permalink / raw)
  To: pshelar, davem, kuba; +Cc: netdev, dev, linux-kernel, 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.

Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
---
 net/openvswitch/actions.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 77d924a..116e38a 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -1238,7 +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;
+	for (a = attr, rem = len; nla_ok(a, rem);
 	     a = nla_next(a, &rem)) {
 		int err = 0;
 
-- 
2.7.4


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] openvswitch: Fix condition check by using nla_ok()
  2021-09-16  1:43 [PATCH] openvswitch: Fix condition check by using nla_ok() Jiasheng Jiang
@ 2021-09-16 14:36 ` Jakub Kicinski
  2021-09-16 14:38   ` Jakub Kicinski
  0 siblings, 1 reply; 4+ messages in thread
From: Jakub Kicinski @ 2021-09-16 14:36 UTC (permalink / raw)
  To: Jiasheng Jiang; +Cc: pshelar, davem, netdev, dev, linux-kernel

On Thu, 16 Sep 2021 01:43:23 +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.
> 
> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>

Are the attributes coming from the user space here or are generated 
by the kernel / were already validated?  Depending on that this is
either a fix and needs to be backported or a possible cleanup.

Please repost with the explanation where attrs come from in the commit
message, and if it's indeed a bug please add a Fixes tag.

If we do need the nla_ok() we should probably also switch to
nla_for_each_attr() and nla_for_each_nested().

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] openvswitch: Fix condition check by using nla_ok()
  2021-09-16 14:36 ` Jakub Kicinski
@ 2021-09-16 14:38   ` Jakub Kicinski
  0 siblings, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2021-09-16 14:38 UTC (permalink / raw)
  To: Jiasheng Jiang; +Cc: pshelar, davem, netdev, dev, linux-kernel

On Thu, 16 Sep 2021 07:36:40 -0700 Jakub Kicinski wrote:
> On Thu, 16 Sep 2021 01:43:23 +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.
> > 
> > Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>  
> 
> Are the attributes coming from the user space here or are generated 
> by the kernel / were already validated?  Depending on that this is
> either a fix and needs to be backported or a possible cleanup.
> 
> Please repost with the explanation where attrs come from in the commit
> message, and if it's indeed a bug please add a Fixes tag.

And please use different subject for each patch, otherwise patchwork
bot thinks this is just two versions of the same patch and marks the 
one posted earlier as Superseded.

> If we do need the nla_ok() we should probably also switch to
> nla_for_each_attr() and nla_for_each_nested().


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH] openvswitch: Fix condition check by using nla_ok()
@ 2021-09-16  1:48 Jiasheng Jiang
  0 siblings, 0 replies; 4+ messages in thread
From: Jiasheng Jiang @ 2021-09-16  1:48 UTC (permalink / raw)
  To: pshelar, davem, kuba; +Cc: netdev, dev, linux-kernel, 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.

Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
---
 net/openvswitch/actions.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 116e38a..8209ab1 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -915,7 +915,7 @@ static int output_userspace(struct datapath *dp, struct sk_buff *skb,
 	upcall.cmd = OVS_PACKET_CMD_ACTION;
 	upcall.mru = OVS_CB(skb)->mru;
 
-	for (a = nla_data(attr), rem = nla_len(attr); rem > 0;
+	for (a = nla_data(attr), rem = nla_len(attr); nla_ok(a, rem);
 	     a = nla_next(a, &rem)) {
 		switch (nla_type(a)) {
 		case OVS_USERSPACE_ATTR_USERDATA:
-- 
2.7.4


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-09-16 14:38 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-16  1:43 [PATCH] openvswitch: Fix condition check by using nla_ok() Jiasheng Jiang
2021-09-16 14:36 ` Jakub Kicinski
2021-09-16 14:38   ` Jakub Kicinski
2021-09-16  1:48 Jiasheng Jiang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).