netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 1/1] taprio: Add boundary check for sched-entry values
@ 2022-12-12 10:23 Lai Peter Jun Ann
  2022-12-12 20:22 ` Vinicius Costa Gomes
  0 siblings, 1 reply; 2+ messages in thread
From: Lai Peter Jun Ann @ 2022-12-12 10:23 UTC (permalink / raw)
  To: Vinicius Costa Gomes, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
  Cc: netdev, linux-kernel, Muhammad Husaini Zulkifli,
	Michael Sit Wei Hong, Lai Peter Jun Ann

From: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>

Adds boundary checks for the gatemask provided against the number of
traffic class defined and the interval times for each sched-entry.

Without this check, the user would not know that the gatemask provided is
invalid and the driver has already truncated the gatemask provided to
match the number of traffic class defined.

The interval times is also checked for values less than 0 or for invalid
inputs such as 00000.

Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>
Signed-off-by: Lai Peter Jun Ann <jun.ann.lai@intel.com>
---
 net/sched/sch_taprio.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 570389f..76a461d 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -786,7 +786,8 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer)
 
 static int fill_sched_entry(struct taprio_sched *q, struct nlattr **tb,
 			    struct sched_entry *entry,
-			    struct netlink_ext_ack *extack)
+			    struct netlink_ext_ack *extack,
+			    u8 num_tc)
 {
 	int min_duration = length_to_duration(q, ETH_ZLEN);
 	u32 interval = 0;
@@ -806,11 +807,16 @@ static int fill_sched_entry(struct taprio_sched *q, struct nlattr **tb,
 	/* The interval should allow at least the minimum ethernet
 	 * frame to go out.
 	 */
-	if (interval < min_duration) {
+	if (interval < min_duration || !interval) {
 		NL_SET_ERR_MSG(extack, "Invalid interval for schedule entry");
 		return -EINVAL;
 	}
 
+	if (entry->gate_mask >= BIT_MASK(num_tc)) {
+		NL_SET_ERR_MSG(extack, "Traffic Class defined less than gatemask");
+		return -EINVAL;
+	}
+
 	entry->interval = interval;
 
 	return 0;
@@ -818,7 +824,8 @@ static int fill_sched_entry(struct taprio_sched *q, struct nlattr **tb,
 
 static int parse_sched_entry(struct taprio_sched *q, struct nlattr *n,
 			     struct sched_entry *entry, int index,
-			     struct netlink_ext_ack *extack)
+			     struct netlink_ext_ack *extack,
+			     u8 num_tc)
 {
 	struct nlattr *tb[TCA_TAPRIO_SCHED_ENTRY_MAX + 1] = { };
 	int err;
@@ -832,12 +839,13 @@ static int parse_sched_entry(struct taprio_sched *q, struct nlattr *n,
 
 	entry->index = index;
 
-	return fill_sched_entry(q, tb, entry, extack);
+	return fill_sched_entry(q, tb, entry, extack, num_tc);
 }
 
 static int parse_sched_list(struct taprio_sched *q, struct nlattr *list,
 			    struct sched_gate_list *sched,
-			    struct netlink_ext_ack *extack)
+			    struct netlink_ext_ack *extack,
+			    u8 num_tc)
 {
 	struct nlattr *n;
 	int err, rem;
@@ -860,7 +868,7 @@ static int parse_sched_list(struct taprio_sched *q, struct nlattr *list,
 			return -ENOMEM;
 		}
 
-		err = parse_sched_entry(q, n, entry, i, extack);
+		err = parse_sched_entry(q, n, entry, i, extack, num_tc);
 		if (err < 0) {
 			kfree(entry);
 			return err;
@@ -877,7 +885,8 @@ static int parse_sched_list(struct taprio_sched *q, struct nlattr *list,
 
 static int parse_taprio_schedule(struct taprio_sched *q, struct nlattr **tb,
 				 struct sched_gate_list *new,
-				 struct netlink_ext_ack *extack)
+				 struct netlink_ext_ack *extack,
+				 u8 num_tc)
 {
 	int err = 0;
 
@@ -897,7 +906,7 @@ static int parse_taprio_schedule(struct taprio_sched *q, struct nlattr **tb,
 
 	if (tb[TCA_TAPRIO_ATTR_SCHED_ENTRY_LIST])
 		err = parse_sched_list(q, tb[TCA_TAPRIO_ATTR_SCHED_ENTRY_LIST],
-				       new, extack);
+				       new, extack, num_tc);
 	if (err < 0)
 		return err;
 
@@ -1541,14 +1550,17 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 	unsigned long flags;
 	ktime_t start;
 	int i, err;
+	u8 num_tc = 0;
 
 	err = nla_parse_nested_deprecated(tb, TCA_TAPRIO_ATTR_MAX, opt,
 					  taprio_policy, extack);
 	if (err < 0)
 		return err;
 
-	if (tb[TCA_TAPRIO_ATTR_PRIOMAP])
+	if (tb[TCA_TAPRIO_ATTR_PRIOMAP]) {
 		mqprio = nla_data(tb[TCA_TAPRIO_ATTR_PRIOMAP]);
+		num_tc = mqprio->num_tc;
+	}
 
 	err = taprio_new_flags(tb[TCA_TAPRIO_ATTR_FLAGS],
 			       q->flags, extack);
@@ -1585,7 +1597,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 		goto free_sched;
 	}
 
-	err = parse_taprio_schedule(q, tb, new_admin, extack);
+	err = parse_taprio_schedule(q, tb, new_admin, extack, num_tc);
 	if (err < 0)
 		goto free_sched;
 
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH net-next 1/1] taprio: Add boundary check for sched-entry values
  2022-12-12 10:23 [PATCH net-next 1/1] taprio: Add boundary check for sched-entry values Lai Peter Jun Ann
@ 2022-12-12 20:22 ` Vinicius Costa Gomes
  0 siblings, 0 replies; 2+ messages in thread
From: Vinicius Costa Gomes @ 2022-12-12 20:22 UTC (permalink / raw)
  To: Lai Peter Jun Ann, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
  Cc: netdev, linux-kernel, Muhammad Husaini Zulkifli,
	Michael Sit Wei Hong, Lai Peter Jun Ann

Lai Peter Jun Ann <jun.ann.lai@intel.com> writes:

> From: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>
>
> Adds boundary checks for the gatemask provided against the number of
> traffic class defined and the interval times for each sched-entry.
>
> Without this check, the user would not know that the gatemask provided is
> invalid and the driver has already truncated the gatemask provided to
> match the number of traffic class defined.
>
> The interval times is also checked for values less than 0 or for invalid
> inputs such as 00000.
>
> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>
> Signed-off-by: Lai Peter Jun Ann <jun.ann.lai@intel.com>
> ---
>  net/sched/sch_taprio.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 570389f..76a461d 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -786,7 +786,8 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer)
>  
>  static int fill_sched_entry(struct taprio_sched *q, struct nlattr **tb,
>  			    struct sched_entry *entry,
> -			    struct netlink_ext_ack *extack)
> +			    struct netlink_ext_ack *extack,
> +			    u8 num_tc)
>  {
>  	int min_duration = length_to_duration(q, ETH_ZLEN);
>  	u32 interval = 0;
> @@ -806,11 +807,16 @@ static int fill_sched_entry(struct taprio_sched *q, struct nlattr **tb,
>  	/* The interval should allow at least the minimum ethernet
>  	 * frame to go out.
>  	 */
> -	if (interval < min_duration) {
> +	if (interval < min_duration || !interval) {

In what situations the case that interval is zero is not handled by
comparing against 'min_duration'? Is 'min_duration' already less than zero
in the cases you are considering? If that's true, that might mean some
other issue.

Also, this looks like a separate patch.

>  		NL_SET_ERR_MSG(extack, "Invalid interval for schedule entry");
>  		return -EINVAL;
>  	}
>  
> +	if (entry->gate_mask >= BIT_MASK(num_tc)) {
> +		NL_SET_ERR_MSG(extack, "Traffic Class defined less than gatemask");
> +		return -EINVAL;
> +	}
> +

Given the amount of gymnastics that you had to do to make this
information get here, I am considering that adding 'num_tc' to
'taprio_sched' might be a good idea.

And now that I think about it, that sounds better indeed, there are
cases where you are able to add an 'admin' schedule without specifying
any priomap (as it won't change between schedules). Your patch looks
like will give an error for that case as num_tc will be always zero.

After that 'num_tc' patch, it might be nice to add a third patch to the
series that would replace the usages of 'netdev_get_num_tc()' by
'q->num_tc' or something like that.

Does it make sense?

>  	entry->interval = interval;
>  
>  	return 0;
> @@ -818,7 +824,8 @@ static int fill_sched_entry(struct taprio_sched *q, struct nlattr **tb,
>  
>  static int parse_sched_entry(struct taprio_sched *q, struct nlattr *n,
>  			     struct sched_entry *entry, int index,
> -			     struct netlink_ext_ack *extack)
> +			     struct netlink_ext_ack *extack,
> +			     u8 num_tc)
>  {
>  	struct nlattr *tb[TCA_TAPRIO_SCHED_ENTRY_MAX + 1] = { };
>  	int err;
> @@ -832,12 +839,13 @@ static int parse_sched_entry(struct taprio_sched *q, struct nlattr *n,
>  
>  	entry->index = index;
>  
> -	return fill_sched_entry(q, tb, entry, extack);
> +	return fill_sched_entry(q, tb, entry, extack, num_tc);
>  }
>  
>  static int parse_sched_list(struct taprio_sched *q, struct nlattr *list,
>  			    struct sched_gate_list *sched,
> -			    struct netlink_ext_ack *extack)
> +			    struct netlink_ext_ack *extack,
> +			    u8 num_tc)
>  {
>  	struct nlattr *n;
>  	int err, rem;
> @@ -860,7 +868,7 @@ static int parse_sched_list(struct taprio_sched *q, struct nlattr *list,
>  			return -ENOMEM;
>  		}
>  
> -		err = parse_sched_entry(q, n, entry, i, extack);
> +		err = parse_sched_entry(q, n, entry, i, extack, num_tc);
>  		if (err < 0) {
>  			kfree(entry);
>  			return err;
> @@ -877,7 +885,8 @@ static int parse_sched_list(struct taprio_sched *q, struct nlattr *list,
>  
>  static int parse_taprio_schedule(struct taprio_sched *q, struct nlattr **tb,
>  				 struct sched_gate_list *new,
> -				 struct netlink_ext_ack *extack)
> +				 struct netlink_ext_ack *extack,
> +				 u8 num_tc)
>  {
>  	int err = 0;
>  
> @@ -897,7 +906,7 @@ static int parse_taprio_schedule(struct taprio_sched *q, struct nlattr **tb,
>  
>  	if (tb[TCA_TAPRIO_ATTR_SCHED_ENTRY_LIST])
>  		err = parse_sched_list(q, tb[TCA_TAPRIO_ATTR_SCHED_ENTRY_LIST],
> -				       new, extack);
> +				       new, extack, num_tc);
>  	if (err < 0)
>  		return err;
>  
> @@ -1541,14 +1550,17 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>  	unsigned long flags;
>  	ktime_t start;
>  	int i, err;
> +	u8 num_tc = 0;
>  
>  	err = nla_parse_nested_deprecated(tb, TCA_TAPRIO_ATTR_MAX, opt,
>  					  taprio_policy, extack);
>  	if (err < 0)
>  		return err;
>  
> -	if (tb[TCA_TAPRIO_ATTR_PRIOMAP])
> +	if (tb[TCA_TAPRIO_ATTR_PRIOMAP]) {
>  		mqprio = nla_data(tb[TCA_TAPRIO_ATTR_PRIOMAP]);
> +		num_tc = mqprio->num_tc;
> +	}

I would prefer that you only did this assignment after 'mqprio' was
properly parsed and validated.

>  
>  	err = taprio_new_flags(tb[TCA_TAPRIO_ATTR_FLAGS],
>  			       q->flags, extack);
> @@ -1585,7 +1597,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>  		goto free_sched;
>  	}
>  
> -	err = parse_taprio_schedule(q, tb, new_admin, extack);
> +	err = parse_taprio_schedule(q, tb, new_admin, extack, num_tc);
>  	if (err < 0)
>  		goto free_sched;
>  
> -- 
> 1.9.1
>


Cheers,
-- 
Vinicius

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-12-12 20:23 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-12 10:23 [PATCH net-next 1/1] taprio: Add boundary check for sched-entry values Lai Peter Jun Ann
2022-12-12 20:22 ` Vinicius Costa Gomes

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).