Message ID | 1631756603-3706451-1-git-send-email-jiasheng@iscas.ac.cn (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | openvswitch: Fix condition check by using nla_ok() | expand |
Context | Check | Description |
---|---|---|
netdev/cover_letter | success | Link |
netdev/fixes_present | success | Link |
netdev/patch_count | success | Link |
netdev/tree_selection | success | Guessed tree name to be net-next |
netdev/subject_prefix | warning | Target tree name not specified in the subject |
netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
netdev/source_inline | success | Was 0 now: 0 |
netdev/verify_signedoff | success | Link |
netdev/module_param | success | Was 0 now: 0 |
netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
netdev/verify_fixes | success | Link |
netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
netdev/header_inline | success | Link |
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().
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().
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;
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(-)