diff mbox series

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

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

Commit Message

Lin Ma July 23, 2023, 7:56 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 attribute has enough length.

Fixes: 4e8b86c06269 ("mqprio: Introduce new hardware offload mode and shaper in mqprio")
Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
 net/sched/sch_mqprio.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Victor Nogueira July 23, 2023, 9:41 p.m. UTC | #1 
On 23/07/2023 04:56, 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 attribute has enough length.
> 
> Fixes: 4e8b86c06269 ("mqprio: Introduce new hardware offload mode and shaper in mqprio")
> Signed-off-by: Lin Ma <linma@zju.edu.cn>
> ---
>   net/sched/sch_mqprio.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
> index ab69ff7577fc..7bd1d261d8e7 100644
> --- a/net/sched/sch_mqprio.c
> +++ b/net/sched/sch_mqprio.c
> @@ -285,7 +285,8 @@ static int mqprio_parse_nlattr(struct Qdisc *sch, struct tc_mqprio_qopt *qopt,
>   		i = 0;
>   		nla_for_each_nested(attr, tb[TCA_MQPRIO_MIN_RATE64],
>   				    rem) {
> -			if (nla_type(attr) != TCA_MQPRIO_MIN_RATE64) {
> +			if (nla_type(attr) != TCA_MQPRIO_MIN_RATE64 ||
> +			    nla_len(attr) < sizeof(u64)) {

Shouldn't the check be nla_len(attr) != sizeof(u64)?
An attribute with a bigger length also seems to be invalid.

You could also separate this check into another if statement,
so that the error message is clearer in regards to why the attr is
invalid. Something like:

if (nla_len(attr) != sizeof(u64)) {
         NL_SET_ERR_MSG_ATTR_FMT(extack, attr,
                                 "Attribute length expected to be %lu",
                                 sizeof(u64));
         return -EINVAL;
}

>   				NL_SET_ERR_MSG_ATTR(extack, attr,
>   						    "Attribute type expected to be TCA_MQPRIO_MIN_RATE64");
>   				return -EINVAL;
> @@ -307,7 +308,8 @@ static int mqprio_parse_nlattr(struct Qdisc *sch, struct tc_mqprio_qopt *qopt,
>   		i = 0;
>   		nla_for_each_nested(attr, tb[TCA_MQPRIO_MAX_RATE64],
>   				    rem) {
> -			if (nla_type(attr) != TCA_MQPRIO_MAX_RATE64) {
> +			if (nla_type(attr) != TCA_MQPRIO_MAX_RATE64 ||
> +			    nla_len(attr) < sizeof(u64)) {

Same as the previous comment.

>   				NL_SET_ERR_MSG_ATTR(extack, attr,
>   						    "Attribute type expected to be TCA_MQPRIO_MAX_RATE64");
>   				return -EINVAL;

cheers,
Victor
Lin Ma July 24, 2023, 1:19 a.m. UTC | #2 
Hello Victor,

>
> Shouldn't the check be nla_len(attr) != sizeof(u64)?
> An attribute with a bigger length also seems to be invalid.
> 
> You could also separate this check into another if statement,
> so that the error message is clearer in regards to why the attr is
> invalid. Something like:
> 
> if (nla_len(attr) != sizeof(u64)) {
>          NL_SET_ERR_MSG_ATTR_FMT(extack, attr,
>                                  "Attribute length expected to be %lu",
>                                  sizeof(u64));
>          return -EINVAL;
> }
> 
> >   				NL_SET_ERR_MSG_ATTR(extack, attr,
> >   						    "Attribute type expected to be TCA_MQPRIO_MIN_RATE64");
> >   				return -EINVAL;
> > @@ -307,7 +308,8 @@ static int mqprio_parse_nlattr(struct Qdisc *sch, struct tc_mqprio_qopt *qopt,
> >   		i = 0;
> >   		nla_for_each_nested(attr, tb[TCA_MQPRIO_MAX_RATE64],
> >   				    rem) {
> > -			if (nla_type(attr) != TCA_MQPRIO_MAX_RATE64) {
> > +			if (nla_type(attr) != TCA_MQPRIO_MAX_RATE64 ||
> > +			    nla_len(attr) < sizeof(u64)) {
> 
> Same as the previous comment.
> 
> >   				NL_SET_ERR_MSG_ATTR(extack, attr,
> >   						    "Attribute type expected to be TCA_MQPRIO_MAX_RATE64");
> >   				return -EINVAL;
> 
> cheers,
> Victor

Yeah, I use < instead of != for a looser check. I agree with you the "!=" condition and the separation suggestion.
I will prepare the v2 ASAP.

Thanks
Lin
diff mbox series

Patch

diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
index ab69ff7577fc..7bd1d261d8e7 100644
--- a/net/sched/sch_mqprio.c
+++ b/net/sched/sch_mqprio.c
@@ -285,7 +285,8 @@  static int mqprio_parse_nlattr(struct Qdisc *sch, struct tc_mqprio_qopt *qopt,
 		i = 0;
 		nla_for_each_nested(attr, tb[TCA_MQPRIO_MIN_RATE64],
 				    rem) {
-			if (nla_type(attr) != TCA_MQPRIO_MIN_RATE64) {
+			if (nla_type(attr) != TCA_MQPRIO_MIN_RATE64 ||
+			    nla_len(attr) < sizeof(u64)) {
 				NL_SET_ERR_MSG_ATTR(extack, attr,
 						    "Attribute type expected to be TCA_MQPRIO_MIN_RATE64");
 				return -EINVAL;
@@ -307,7 +308,8 @@  static int mqprio_parse_nlattr(struct Qdisc *sch, struct tc_mqprio_qopt *qopt,
 		i = 0;
 		nla_for_each_nested(attr, tb[TCA_MQPRIO_MAX_RATE64],
 				    rem) {
-			if (nla_type(attr) != TCA_MQPRIO_MAX_RATE64) {
+			if (nla_type(attr) != TCA_MQPRIO_MAX_RATE64 ||
+			    nla_len(attr) < sizeof(u64)) {
 				NL_SET_ERR_MSG_ATTR(extack, attr,
 						    "Attribute type expected to be TCA_MQPRIO_MAX_RATE64");
 				return -EINVAL;