| Message ID | 20230724014625.4087030-1-linma@zju.edu.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net/sched: mqprio: Add length check for TCA_MQPRIO_{MAX/MIN}_RATE64 | expand |
On 23/07/2023 22:46, Lin Ma wrote: > The nla_for_each_nested parsing in function mqprio_parse_nlattr() does > not check the length of the nested attribute. This can lead to an > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > be viewed as 8 byte integer and passed to priv->max_rate/min_rate. > > This patch adds the check based on nla_len() when check the nla_type(), > which ensures that the length of these two attribute must equals > sizeof(u64). > > Fixes: 4e8b86c06269 ("mqprio: Introduce new hardware offload mode and shaper in mqprio") > Signed-off-by: Lin Ma <linma@zju.edu.cn> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
On Mon, 24 Jul 2023 09:46:25 +0800 Lin Ma wrote: > The nla_for_each_nested parsing in function mqprio_parse_nlattr() does > not check the length of the nested attribute. This can lead to an > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > be viewed as 8 byte integer and passed to priv->max_rate/min_rate. > > This patch adds the check based on nla_len() when check the nla_type(), > which ensures that the length of these two attribute must equals > sizeof(u64). How do you run get_maintainer? You didn't CC the author of the code.
Hello Jakub, > > The nla_for_each_nested parsing in function mqprio_parse_nlattr() does > > not check the length of the nested attribute. This can lead to an > > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > > be viewed as 8 byte integer and passed to priv->max_rate/min_rate. > > > > This patch adds the check based on nla_len() when check the nla_type(), > > which ensures that the length of these two attribute must equals > > sizeof(u64). > > How do you run get_maintainer? You didn't CC the author of the code. That's weird, I just ran code below and send this patch to all 9 emails poped out. # ./scripts/get_maintainer.pl net/sched/sch_mqprio.c Jamal Hadi Salim <jhs@mojatatu.com> (maintainer:TC subsystem) Cong Wang <xiyou.wangcong@gmail.com> (maintainer:TC subsystem) Jiri Pirko <jiri@resnulli.us> (maintainer:TC subsystem) "David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING [GENERAL]) Eric Dumazet <edumazet@google.com> (maintainer:NETWORKING [GENERAL]) Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING [GENERAL]) Paolo Abeni <pabeni@redhat.com> (maintainer:NETWORKING [GENERAL]) netdev@vger.kernel.org (open list:TC subsystem) linux-kernel@vger.kernel.org (open list) Can you tell me which one is missing and I will resend the patch to him. Thanks Lin
On Tue, 25 Jul 2023 08:15:39 +0800 (GMT+08:00) Lin Ma wrote: > > > The nla_for_each_nested parsing in function mqprio_parse_nlattr() does > > > not check the length of the nested attribute. This can lead to an > > > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > > > be viewed as 8 byte integer and passed to priv->max_rate/min_rate. > > > > > > This patch adds the check based on nla_len() when check the nla_type(), > > > which ensures that the length of these two attribute must equals > > > sizeof(u64). > > > > How do you run get_maintainer? You didn't CC the author of the code. > > That's weird, I just ran code below and send this patch to all 9 emails poped out. > > # ./scripts/get_maintainer.pl net/sched/sch_mqprio.c Joe, here's another case. Lin Ma, you need to run the script on the file generated by git format-patch, rather than the file path. That gives better coverage for keywords included in the commit message (especially the Fixes tag). Please rerun it on the patch and repost with the right CC list.
Hi Jakub, > > > > The nla_for_each_nested parsing in function mqprio_parse_nlattr() does > > > > not check the length of the nested attribute. This can lead to an > > > > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > > > > be viewed as 8 byte integer and passed to priv->max_rate/min_rate. > > > > > > > > This patch adds the check based on nla_len() when check the nla_type(), > > > > which ensures that the length of these two attribute must equals > > > > sizeof(u64). > > > > > > How do you run get_maintainer? You didn't CC the author of the code. > > > > That's weird, I just ran code below and send this patch to all 9 emails poped out. > > > > # ./scripts/get_maintainer.pl net/sched/sch_mqprio.c > > Joe, here's another case. > > Lin Ma, you need to run the script on the file generated by > git format-patch, rather than the file path. That gives better > coverage for keywords included in the commit message (especially > the Fixes tag). Please rerun it on the patch and repost with > the right CC list. Copy that. Sorry for the inconvenience that was raised by that. Will resend the patch with the correct CC list ASAP. Regards Lin > -- > pw-bot: cr
On Mon, 2023-07-24 at 17:56 -0700, Jakub Kicinski wrote: > On Tue, 25 Jul 2023 08:15:39 +0800 (GMT+08:00) Lin Ma wrote: > > > > The nla_for_each_nested parsing in function mqprio_parse_nlattr() does > > > > not check the length of the nested attribute. This can lead to an > > > > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > > > > be viewed as 8 byte integer and passed to priv->max_rate/min_rate. > > > > > > > > This patch adds the check based on nla_len() when check the nla_type(), > > > > which ensures that the length of these two attribute must equals > > > > sizeof(u64). > > > > > > How do you run get_maintainer? You didn't CC the author of the code. > > > > That's weird, I just ran code below and send this patch to all 9 emails poped out. > > > > # ./scripts/get_maintainer.pl net/sched/sch_mqprio.c > > Joe, here's another case. What do you think the "case" is here? Do you think John Fastabend, who hasn't touched the file in 7+ years should be cc'd? Why? > Lin Ma, you need to run the script on the file generated by > git format-patch, rather than the file path. That gives better > coverage for keywords included in the commit message (especially > the Fixes tag). Please rerun it on the patch and repost with > the right CC list.
On Mon, 24 Jul 2023 20:59:53 -0700 Joe Perches wrote: > > Joe, here's another case. > > What do you think the "case" is here? > > Do you think John Fastabend, who hasn't touched the file in 7+ years > should be cc'd? Why? Nope. The author of the patch under Fixes.
On Tue, 2023-07-25 at 12:38 -0700, Jakub Kicinski wrote: > On Mon, 24 Jul 2023 20:59:53 -0700 Joe Perches wrote: > > > Joe, here's another case. > > > > What do you think the "case" is here? > > > > Do you think John Fastabend, who hasn't touched the file in 7+ years > > should be cc'd? Why? > > Nope. The author of the patch under Fixes. It adds that already since 2019. commit 2f5bd343694ed53b3abc4a616ce975505271afe7 Author: Joe Perches <joe@perches.com> Date: Wed Dec 4 16:50:29 2019 -0800 scripts/get_maintainer.pl: add signatures from Fixes: <badcommit> lines in commit message A Fixes: lines in a commit message generally indicate that a previous commit was inadequate for whatever reason. The signers of the previous inadequate commit should also be cc'd on this new commit so update get_maintainer to find the old commit and add the original signers.
On Tue, 25 Jul 2023 12:50:00 -0700 Joe Perches wrote: > > > What do you think the "case" is here? > > > > > > Do you think John Fastabend, who hasn't touched the file in 7+ years > > > should be cc'd? Why? > > > > Nope. The author of the patch under Fixes. > > It adds that already since 2019. Which is awesome! But for that to work you have to run get_maintainer on the patchfile not the file paths. Lin Ma used: # ./scripts/get_maintainer.pl net/sched/sch_mqprio.c That's what I keep complaining about. People run get_maintainer on paths and miss out on all the get_maintainer features which need to see the commit message.
On Tue, 2023-07-25 at 12:56 -0700, Jakub Kicinski wrote: > On Tue, 25 Jul 2023 12:50:00 -0700 Joe Perches wrote: > > > > What do you think the "case" is here? > > > > > > > > Do you think John Fastabend, who hasn't touched the file in 7+ years > > > > should be cc'd? Why? > > > > > > Nope. The author of the patch under Fixes. > > > > It adds that already since 2019. > > Which is awesome! But for that to work you have to run get_maintainer > on the patchfile not the file paths. Lin Ma used: > > # ./scripts/get_maintainer.pl net/sched/sch_mqprio.c > > That's what I keep complaining about. People run get_maintainer on > paths and miss out on all the get_maintainer features which need > to see the commit message. Which is useful when editing a file but not when sending a patch. get_maintainer does _both_. Again, there's no issue with get_maintainer, but there is in its use. If there's a real issue, it's with people and their knowledge of process documentation. It's _not_ the tool itself as you stated.
diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c index ab69ff7577fc..f1d141a6d0aa 100644 --- a/net/sched/sch_mqprio.c +++ b/net/sched/sch_mqprio.c @@ -290,6 +290,13 @@ static int mqprio_parse_nlattr(struct Qdisc *sch, struct tc_mqprio_qopt *qopt, "Attribute type expected to be TCA_MQPRIO_MIN_RATE64"); return -EINVAL; } + + if (nla_len(attr) != sizeof(u64)) { + NL_SET_ERR_MSG_ATTR(extack, attr, + "Attribute TCA_MQPRIO_MIN_RATE64 expected to have 8 bytes length"); + return -EINVAL; + } + if (i >= qopt->num_tc) break; priv->min_rate[i] = nla_get_u64(attr); @@ -312,6 +319,13 @@ static int mqprio_parse_nlattr(struct Qdisc *sch, struct tc_mqprio_qopt *qopt, "Attribute type expected to be TCA_MQPRIO_MAX_RATE64"); return -EINVAL; } + + if (nla_len(attr) != sizeof(u64)) { + NL_SET_ERR_MSG_ATTR(extack, attr, + "Attribute TCA_MQPRIO_MAX_RATE64 expected to have 8 bytes length"); + return -EINVAL; + } + if (i >= qopt->num_tc) break; priv->max_rate[i] = nla_get_u64(attr);
The nla_for_each_nested parsing in function mqprio_parse_nlattr() does not check the length of the nested attribute. This can lead to an out-of-attribute read and allow a malformed nlattr (e.g., length 0) to be viewed as 8 byte integer and passed to priv->max_rate/min_rate. This patch adds the check based on nla_len() when check the nla_type(), which ensures that the length of these two attribute must equals sizeof(u64). Fixes: 4e8b86c06269 ("mqprio: Introduce new hardware offload mode and shaper in mqprio") Signed-off-by: Lin Ma <linma@zju.edu.cn> --- V1 -> V2: do check with != rather than < as suggested seperate the check and give clearer error message net/sched/sch_mqprio.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)