| Message ID | 20210114163822.56306-1-xiyou.wangcong@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] 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: 88 this patch: 88 |
| 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, 48 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 88 this patch: 88 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, 14 Jan 2021 08:38:22 -0800 Cong Wang wrote: > From: Cong Wang <cong.wang@bytedance.com> > > fl_set_enc_opt() simply checks if there are still bytes left to parse, > but this is not sufficent as syzbot seems to be able to generate > malformatted netlink messages. nla_ok() is more strict so should be > used to validate the next nlattr here. > > And nla_validate_nested_deprecated() has less strict check too, it is > probably too late to switch to the strict version, but we can just > call nla_ok() too after it. > > Reported-and-tested-by: syzbot+2624e3778b18fc497c92@syzkaller.appspotmail.com > Fixes: 0a6e77784f49 ("net/sched: allow flower to match tunnel options") > Fixes: 79b1011cb33d ("net: sched: allow flower to match erspan options") > @@ -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,17 @@ 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_ok(nla_opt_msk, msk_depth)) { > + NL_SET_ERR_MSG(extack, "Mask attribute is invalid"); > + return -EINVAL; > + } > + nla_opt_msk = nla_next(nla_opt_msk, &msk_depth); we lost the if (msk_depth) now, nla_opt_msk may be NULL - neither nla_ok() nor nla_next() take NULL > } > > return 0;
On Thu, Jan 14, 2021 at 10:38 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 14 Jan 2021 08:38:22 -0800 Cong Wang wrote: > > From: Cong Wang <cong.wang@bytedance.com> > > > > fl_set_enc_opt() simply checks if there are still bytes left to parse, > > but this is not sufficent as syzbot seems to be able to generate > > malformatted netlink messages. nla_ok() is more strict so should be > > used to validate the next nlattr here. > > > > And nla_validate_nested_deprecated() has less strict check too, it is > > probably too late to switch to the strict version, but we can just > > call nla_ok() too after it. > > > > Reported-and-tested-by: syzbot+2624e3778b18fc497c92@syzkaller.appspotmail.com > > Fixes: 0a6e77784f49 ("net/sched: allow flower to match tunnel options") > > Fixes: 79b1011cb33d ("net: sched: allow flower to match erspan options") > > > @@ -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,17 @@ 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_ok(nla_opt_msk, msk_depth)) { > > + NL_SET_ERR_MSG(extack, "Mask attribute is invalid"); > > + return -EINVAL; > > + } > > + nla_opt_msk = nla_next(nla_opt_msk, &msk_depth); > > we lost the if (msk_depth) now, nla_opt_msk may be NULL - > neither nla_ok() nor nla_next() take NULL How is "if (msk_depth)" lost when nla_ok() has a stricter one? 1156 static inline int nla_ok(const struct nlattr *nla, int remaining) 1157 { 1158 return remaining >= (int) sizeof(*nla) && 1159 nla->nla_len >= sizeof(*nla) && 1160 nla->nla_len <= remaining; 1161 } Line 1156 assures msk_depth is not only non-zero but also larger than the nla struct size, and clearly nla won't be dereferenced unless this check is passed. I guess you mean we should not error out for nla_opt_msk==NULL case as masks are optional? Thanks.
On Thu, 14 Jan 2021 12:03:16 -0800 Cong Wang wrote: > On Thu, Jan 14, 2021 at 10:38 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Thu, 14 Jan 2021 08:38:22 -0800 Cong Wang wrote: > > > From: Cong Wang <cong.wang@bytedance.com> > > > > > > fl_set_enc_opt() simply checks if there are still bytes left to parse, > > > but this is not sufficent as syzbot seems to be able to generate > > > malformatted netlink messages. nla_ok() is more strict so should be > > > used to validate the next nlattr here. > > > > > > And nla_validate_nested_deprecated() has less strict check too, it is > > > probably too late to switch to the strict version, but we can just > > > call nla_ok() too after it. > > > > > > Reported-and-tested-by: syzbot+2624e3778b18fc497c92@syzkaller.appspotmail.com > > > Fixes: 0a6e77784f49 ("net/sched: allow flower to match tunnel options") > > > Fixes: 79b1011cb33d ("net: sched: allow flower to match erspan options") > > > > > @@ -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,17 @@ 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_ok(nla_opt_msk, msk_depth)) { > > > + NL_SET_ERR_MSG(extack, "Mask attribute is invalid"); > > > + return -EINVAL; > > > + } > > > + nla_opt_msk = nla_next(nla_opt_msk, &msk_depth); > > > > we lost the if (msk_depth) now, nla_opt_msk may be NULL - > > neither nla_ok() nor nla_next() take NULL > > How is "if (msk_depth)" lost when nla_ok() has a stricter one? > > 1156 static inline int nla_ok(const struct nlattr *nla, int remaining) > 1157 { > 1158 return remaining >= (int) sizeof(*nla) && > 1159 nla->nla_len >= sizeof(*nla) && > 1160 nla->nla_len <= remaining; > 1161 } > > Line 1156 assures msk_depth is not only non-zero but also larger > than the nla struct size, and clearly nla won't be dereferenced unless > this check is passed. Fair, depth will but 0 so first check already fails, but nla_next() would crash since it tries to access the length of the attribute unconditionally. > I guess you mean we should not error out for nla_opt_msk==NULL > case as masks are optional? Yup.
On Thu, Jan 14, 2021 at 12:16 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 14 Jan 2021 12:03:16 -0800 Cong Wang wrote: > > On Thu, Jan 14, 2021 at 10:38 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > > > On Thu, 14 Jan 2021 08:38:22 -0800 Cong Wang wrote: > > > > From: Cong Wang <cong.wang@bytedance.com> > > > > > > > > fl_set_enc_opt() simply checks if there are still bytes left to parse, > > > > but this is not sufficent as syzbot seems to be able to generate > > > > malformatted netlink messages. nla_ok() is more strict so should be > > > > used to validate the next nlattr here. > > > > > > > > And nla_validate_nested_deprecated() has less strict check too, it is > > > > probably too late to switch to the strict version, but we can just > > > > call nla_ok() too after it. > > > > > > > > Reported-and-tested-by: syzbot+2624e3778b18fc497c92@syzkaller.appspotmail.com > > > > Fixes: 0a6e77784f49 ("net/sched: allow flower to match tunnel options") > > > > Fixes: 79b1011cb33d ("net: sched: allow flower to match erspan options") > > > > > > > @@ -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,17 @@ 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_ok(nla_opt_msk, msk_depth)) { > > > > + NL_SET_ERR_MSG(extack, "Mask attribute is invalid"); > > > > + return -EINVAL; > > > > + } > > > > + nla_opt_msk = nla_next(nla_opt_msk, &msk_depth); > > > > > > we lost the if (msk_depth) now, nla_opt_msk may be NULL - > > > neither nla_ok() nor nla_next() take NULL > > > > How is "if (msk_depth)" lost when nla_ok() has a stricter one? > > > > 1156 static inline int nla_ok(const struct nlattr *nla, int remaining) > > 1157 { > > 1158 return remaining >= (int) sizeof(*nla) && > > 1159 nla->nla_len >= sizeof(*nla) && > > 1160 nla->nla_len <= remaining; > > 1161 } > > > > Line 1156 assures msk_depth is not only non-zero but also larger > > than the nla struct size, and clearly nla won't be dereferenced unless > > this check is passed. > > Fair, depth will but 0 so first check already fails, but nla_next() > would crash since it tries to access the length of the attribute > unconditionally. nla_next() is only called when nla_ok() returns true, which is not the case for msk_depth==0, therefore NULL won't crash here. The only problem is we become too strict to reject optionally missing masks, we should not even call nla_ok() here, otherwise it would break user-space. So, + if (!nla_opt_msk) + continue; Thanks.
On Thu, 14 Jan 2021 12:24:19 -0800 Cong Wang wrote: > > Fair, depth will but 0 so first check already fails, but nla_next() > > would crash since it tries to access the length of the attribute > > unconditionally. > > nla_next() is only called when nla_ok() returns true, which is not > the case for msk_depth==0, therefore NULL won't crash here. > > The only problem is we become too strict to reject optionally missing > masks, we should not even call nla_ok() here, otherwise it would > break user-space. So, > > + if (!nla_opt_msk) > + continue; > > Thanks. You're right.
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index 1319986693fc..740d9018e45f 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,17 @@ 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_ok(nla_opt_msk, msk_depth)) { + NL_SET_ERR_MSG(extack, "Mask attribute is invalid"); + return -EINVAL; + } + nla_opt_msk = nla_next(nla_opt_msk, &msk_depth); } return 0;