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 <[email protected]>
---
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(+)
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);
--
2.17.1
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 <[email protected]>
Reviewed-by: Victor Nogueira <[email protected]>
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 <[email protected]> (maintainer:TC subsystem)
Cong Wang <[email protected]> (maintainer:TC subsystem)
Jiri Pirko <[email protected]> (maintainer:TC subsystem)
"David S. Miller" <[email protected]> (maintainer:NETWORKING [GENERAL])
Eric Dumazet <[email protected]> (maintainer:NETWORKING [GENERAL])
Jakub Kicinski <[email protected]> (maintainer:NETWORKING [GENERAL])
Paolo Abeni <[email protected]> (maintainer:NETWORKING [GENERAL])
[email protected] (open list:TC subsystem)
[email protected] (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.
--
pw-bot: cr
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 <[email protected]>
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.