| Message ID | 20210114210749.61642-1-xiyou.wangcong@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] cls_flower: call nla_ok() before nla_next() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 3 maintainers not CCed: pieter.jansenvanvuuren@netronome.com davem@davemloft.net simon.horman@netronome.com |
| 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: 131 this patch: 131 |
| 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, 51 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 123 this patch: 123 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, 14 Jan 2021 13:07:49 -0800 Cong Wang wrote: > - if (msk_depth) > - nla_opt_msk = nla_next(nla_opt_msk, &msk_depth); > break; > default: > NL_SET_ERR_MSG(extack, "Unknown tunnel option type"); > return -EINVAL; > } > + > + if (!nla_opt_msk) > + continue; Why the switch from !msk_depth to !nla_opt_msk? Seems like previously providing masks for only subset of options would have worked.
On Thu, Jan 14, 2021 at 1:36 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 14 Jan 2021 13:07:49 -0800 Cong Wang wrote: > > - if (msk_depth) > > - nla_opt_msk = nla_next(nla_opt_msk, &msk_depth); > > break; > > default: > > NL_SET_ERR_MSG(extack, "Unknown tunnel option type"); > > return -EINVAL; > > } > > + > > + if (!nla_opt_msk) > > + continue; > > Why the switch from !msk_depth to !nla_opt_msk? It is the same, when nla_opt_msk is NULL, msk_depth is 0. Checking nla_opt_msk is NULL is more readable to express that mask is not provided. > > Seems like previously providing masks for only subset of options > would have worked. I don't think so, every type has this check: if (key->enc_opts.len != mask->enc_opts.len) { NL_SET_ERR_MSG(extack, "Key and mask miss aligned"); return -EINVAL; } which guarantees the numbers are aligned. Thanks.
On Thu, 14 Jan 2021 13:57:13 -0800 Cong Wang wrote: > On Thu, Jan 14, 2021 at 1:36 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Thu, 14 Jan 2021 13:07:49 -0800 Cong Wang wrote: > > > - if (msk_depth) > > > - nla_opt_msk = nla_next(nla_opt_msk, &msk_depth); > > > break; > > > default: > > > NL_SET_ERR_MSG(extack, "Unknown tunnel option type"); > > > return -EINVAL; > > > } > > > + > > > + if (!nla_opt_msk) > > > + continue; > > > > Why the switch from !msk_depth to !nla_opt_msk? > > It is the same, when nla_opt_msk is NULL, msk_depth is 0. > Checking nla_opt_msk is NULL is more readable to express that > mask is not provided. > > > > > Seems like previously providing masks for only subset of options > > would have worked. > > I don't think so, every type has this check: > > if (key->enc_opts.len != mask->enc_opts.len) { > NL_SET_ERR_MSG(extack, "Key and mask > miss aligned"); > return -EINVAL; > } > > which guarantees the numbers are aligned. > > Thanks. static int fl_set_vxlan_opt(const struct nlattr *nla, struct fl_flow_key *key, int depth, int option_len, struct netlink_ext_ack *extack) { struct nlattr *tb[TCA_FLOWER_KEY_ENC_OPT_VXLAN_MAX + 1]; struct vxlan_metadata *md; int err; md = (struct vxlan_metadata *)&key->enc_opts.data[key->enc_opts.len]; memset(md, 0xff, sizeof(*md)); if (!depth) return sizeof(*md); ^^^^^^^^^^^^^^^^^^^ The mask is filled with all 1s if attribute is not provided.
On Thu, Jan 14, 2021 at 2:30 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 14 Jan 2021 13:57:13 -0800 Cong Wang wrote: > > On Thu, Jan 14, 2021 at 1:36 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > > > On Thu, 14 Jan 2021 13:07:49 -0800 Cong Wang wrote: > > > > - if (msk_depth) > > > > - nla_opt_msk = nla_next(nla_opt_msk, &msk_depth); > > > > break; > > > > default: > > > > NL_SET_ERR_MSG(extack, "Unknown tunnel option type"); > > > > return -EINVAL; > > > > } > > > > + > > > > + if (!nla_opt_msk) > > > > + continue; > > > > > > Why the switch from !msk_depth to !nla_opt_msk? > > > > It is the same, when nla_opt_msk is NULL, msk_depth is 0. > > Checking nla_opt_msk is NULL is more readable to express that > > mask is not provided. > > > > > > > > Seems like previously providing masks for only subset of options > > > would have worked. > > > > I don't think so, every type has this check: > > > > if (key->enc_opts.len != mask->enc_opts.len) { > > NL_SET_ERR_MSG(extack, "Key and mask > > miss aligned"); > > return -EINVAL; > > } > > > > which guarantees the numbers are aligned. > > > > Thanks. > > static int fl_set_vxlan_opt(const struct nlattr *nla, struct fl_flow_key *key, > int depth, int option_len, > struct netlink_ext_ack *extack) > { > struct nlattr *tb[TCA_FLOWER_KEY_ENC_OPT_VXLAN_MAX + 1]; > struct vxlan_metadata *md; > int err; > > md = (struct vxlan_metadata *)&key->enc_opts.data[key->enc_opts.len]; > memset(md, 0xff, sizeof(*md)); > > if (!depth) > return sizeof(*md); > ^^^^^^^^^^^^^^^^^^^ > > The mask is filled with all 1s if attribute is not provided. Hmm, then what is the length comparison check for? fl_set_vxlan_opt() either turns negative or sizeof(*md), and negitve is already checked when it returns, so when we hit the length comparison it is always equal. So it must be redundant. (Note, I am only talking about the vxlan case you pick here, because the geneve case is different, as it returns different sizes.) Thanks.
On Thu, 14 Jan 2021 14:50:04 -0800 Cong Wang wrote: > > static int fl_set_vxlan_opt(const struct nlattr *nla, struct fl_flow_key *key, > > int depth, int option_len, > > struct netlink_ext_ack *extack) > > { > > struct nlattr *tb[TCA_FLOWER_KEY_ENC_OPT_VXLAN_MAX + 1]; > > struct vxlan_metadata *md; > > int err; > > > > md = (struct vxlan_metadata *)&key->enc_opts.data[key->enc_opts.len]; > > memset(md, 0xff, sizeof(*md)); > > > > if (!depth) > > return sizeof(*md); > > ^^^^^^^^^^^^^^^^^^^ > > > > The mask is filled with all 1s if attribute is not provided. > > Hmm, then what is the length comparison check for? > > fl_set_vxlan_opt() either turns negative or sizeof(*md), and negitve > is already checked when it returns, so when we hit the length comparison > it is always equal. So it must be redundant. > > (Note, I am only talking about the vxlan case you pick here, because the > geneve case is different, as it returns different sizes.) Good question
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index 1319986693fc..280eab0c833f 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -1272,6 +1272,10 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key, nla_opt_msk = nla_data(tb[TCA_FLOWER_KEY_ENC_OPTS_MASK]); msk_depth = nla_len(tb[TCA_FLOWER_KEY_ENC_OPTS_MASK]); + if (!nla_ok(nla_opt_msk, msk_depth)) { + NL_SET_ERR_MSG(extack, "Invalid attribute for masks"); + return -EINVAL; + } } nla_for_each_attr(nla_opt_key, nla_enc_key, @@ -1307,9 +1311,6 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key, NL_SET_ERR_MSG(extack, "Key and mask miss aligned"); return -EINVAL; } - - if (msk_depth) - nla_opt_msk = nla_next(nla_opt_msk, &msk_depth); break; case TCA_FLOWER_KEY_ENC_OPTS_VXLAN: if (key->enc_opts.dst_opt_type) { @@ -1340,9 +1341,6 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key, NL_SET_ERR_MSG(extack, "Key and mask miss aligned"); return -EINVAL; } - - if (msk_depth) - nla_opt_msk = nla_next(nla_opt_msk, &msk_depth); break; case TCA_FLOWER_KEY_ENC_OPTS_ERSPAN: if (key->enc_opts.dst_opt_type) { @@ -1373,14 +1371,20 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key, NL_SET_ERR_MSG(extack, "Key and mask miss aligned"); return -EINVAL; } - - if (msk_depth) - nla_opt_msk = nla_next(nla_opt_msk, &msk_depth); break; default: NL_SET_ERR_MSG(extack, "Unknown tunnel option type"); return -EINVAL; } + + if (!nla_opt_msk) + continue; + + if (!nla_ok(nla_opt_msk, msk_depth)) { + NL_SET_ERR_MSG(extack, "A mask attribute is invalid"); + return -EINVAL; + } + nla_opt_msk = nla_next(nla_opt_msk, &msk_depth); } return 0;