diff mbox series

[v2] net/sched: mqprio: Add length check for TCA_MQPRIO_{MAX/MIN}_RATE64

Message ID 20230724014625.4087030-1-linma@zju.edu.cn
State New
Headers
Series [v2] net/sched: mqprio: Add length check for TCA_MQPRIO_{MAX/MIN}_RATE64 |

Commit Message

Lin Ma July 24, 2023, 1:46 a.m. UTC 
  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(+)

Comments

Victor Nogueira July 24, 2023, 9:29 p.m. UTC | #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 <linma@zju.edu.cn>

Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Jakub Kicinski July 24, 2023, 11:02 p.m. UTC | #2 
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.
Lin Ma July 25, 2023, 12:15 a.m. UTC | #3 
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
Jakub Kicinski July 25, 2023, 12:56 a.m. UTC | #4 
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.
Lin Ma July 25, 2023, 1:33 a.m. UTC | #5 
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
Joe Perches July 25, 2023, 3:59 a.m. UTC | #6 
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.
Jakub Kicinski July 25, 2023, 7:38 p.m. UTC | #7 
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.
Joe Perches July 25, 2023, 7:50 p.m. UTC | #8 
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.
Jakub Kicinski July 25, 2023, 7:56 p.m. UTC | #9 
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.
Joe Perches July 25, 2023, 9:04 p.m. UTC | #10 
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 mbox series

Patch

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);