[PATCH net-next v2 1/1] taprio: Add boundary check for sched-entry values

[PATCH net-next v2 1/1] taprio: Add boundary check for sched-entry values

[Lai Peter Jun Ann]

Adds boundary checks for the gatemask provided against the number of
traffic class defined 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.

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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 1cb5e41..44b9e21 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -102,6 +102,7 @@ struct taprio_sched {
 	u32 max_sdu[TC_MAX_QUEUE]; /* save info from the user */
 	u32 fp[TC_QOPT_MAX_QUEUE]; /* only for dump and offloading */
 	u32 txtime_delay;
+	u8 num_tc;
 };
 
 struct __tc_taprio_qopt_offload {
@@ -1063,6 +1064,11 @@ static int fill_sched_entry(struct taprio_sched *q, struct nlattr **tb,
 		return -EINVAL;
 	}
 
+	if (entry->gate_mask >= q->num_tc) {
+		NL_SET_ERR_MSG(extack, "Traffic Class defined less than gatemask");
+		return -EINVAL;
+	}
+
 	entry->interval = interval;
 
 	return 0;
@@ -1913,6 +1919,8 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 		for (i = 0; i <= TC_BITMASK; i++)
 			netdev_set_prio_tc_map(dev, i,
 					       mqprio->prio_tc_map[i]);
+
+		q->num_tc = mqprio->num_tc;
 	}
 
 	err = parse_taprio_schedule(q, tb, new_admin, extack);
-- 
1.9.1

Re: [PATCH net-next v2 1/1] taprio: Add boundary check for sched-entry values

[Gerhard Engleder]

On 18.10.23 05:28, Lai Peter Jun Ann wrote:
> Adds boundary checks for the gatemask provided against the number of
> traffic class defined 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.
> 
> 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 | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 1cb5e41..44b9e21 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -102,6 +102,7 @@ struct taprio_sched {
>   	u32 max_sdu[TC_MAX_QUEUE]; /* save info from the user */
>   	u32 fp[TC_QOPT_MAX_QUEUE]; /* only for dump and offloading */
>   	u32 txtime_delay;
> +	u8 num_tc;
>   };
>   
>   struct __tc_taprio_qopt_offload {
> @@ -1063,6 +1064,11 @@ static int fill_sched_entry(struct taprio_sched *q, struct nlattr **tb,
>   		return -EINVAL;
>   	}
>   
> +	if (entry->gate_mask >= q->num_tc) {

As far as I know within gate_mask every bit represents a traffic class.
So for 3 traffic classes at gate_mask of 0x7 is valid but this check
fails with 0x7 >= 3.

> +		NL_SET_ERR_MSG(extack, "Traffic Class defined less than gatemask");
> +		return -EINVAL;
> +	}
> +
>   	entry->interval = interval;
>   
>   	return 0;
> @@ -1913,6 +1919,8 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>   		for (i = 0; i <= TC_BITMASK; i++)
>   			netdev_set_prio_tc_map(dev, i,
>   					       mqprio->prio_tc_map[i]);
> +
> +		q->num_tc = mqprio->num_tc;
>   	}
>   
>   	err = parse_taprio_schedule(q, tb, new_admin, extack);

Re: [PATCH net-next v2 1/1] taprio: Add boundary check for sched-entry values

[Paolo Abeni]

On Wed, 2023-10-18 at 19:56 +0200, Gerhard Engleder wrote:
> On 18.10.23 05:28, Lai Peter Jun Ann wrote:
> > Adds boundary checks for the gatemask provided against the number of
> > traffic class defined 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.
> > 
> > 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 | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> > 
> > diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> > index 1cb5e41..44b9e21 100644
> > --- a/net/sched/sch_taprio.c
> > +++ b/net/sched/sch_taprio.c
> > @@ -102,6 +102,7 @@ struct taprio_sched {
> >   	u32 max_sdu[TC_MAX_QUEUE]; /* save info from the user */
> >   	u32 fp[TC_QOPT_MAX_QUEUE]; /* only for dump and offloading */
> >   	u32 txtime_delay;
> > +	u8 num_tc;
> >   };
> >   
> >   struct __tc_taprio_qopt_offload {
> > @@ -1063,6 +1064,11 @@ static int fill_sched_entry(struct taprio_sched *q, struct nlattr **tb,
> >   		return -EINVAL;
> >   	}
> >   
> > +	if (entry->gate_mask >= q->num_tc) {
> 
> As far as I know within gate_mask every bit represents a traffic class.
> So for 3 traffic classes at gate_mask of 0x7 is valid but this check
> fails with 0x7 >= 3.

Additionally whatever check we put in place previously just ignored by
the existing code, could break the existing user-space: we can't accept
such change. 

Cheers,

Paolo

Re: [PATCH net-next v2 1/1] taprio: Add boundary check for sched-entry values

[Vladimir Oltean]

On Thu, Oct 19, 2023 at 12:35:30PM +0200, Paolo Abeni wrote:
> On Wed, 2023-10-18 at 19:56 +0200, Gerhard Engleder wrote:
> > On 18.10.23 05:28, Lai Peter Jun Ann wrote:
> > > Adds boundary checks for the gatemask provided against the number of
> > > traffic class defined 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.
> > > 
> > > 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 | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> > > index 1cb5e41..44b9e21 100644
> > > --- a/net/sched/sch_taprio.c
> > > +++ b/net/sched/sch_taprio.c
> > > @@ -102,6 +102,7 @@ struct taprio_sched {
> > >   	u32 max_sdu[TC_MAX_QUEUE]; /* save info from the user */
> > >   	u32 fp[TC_QOPT_MAX_QUEUE]; /* only for dump and offloading */
> > >   	u32 txtime_delay;
> > > +	u8 num_tc;

To the patch: I would oppose introducing an "u8 num_tc" to struct
taprio_sched for one purpose only. It is a duplication of
netdev->num_tc, the only problem is that it hasn't yet been set, which
can be solved with a bit of code reorganization.

> > >   };
> > >   
> > >   struct __tc_taprio_qopt_offload {
> > > @@ -1063,6 +1064,11 @@ static int fill_sched_entry(struct taprio_sched *q, struct nlattr **tb,
> > >   		return -EINVAL;
> > >   	}
> > >   
> > > +	if (entry->gate_mask >= q->num_tc) {
> > 
> > As far as I know within gate_mask every bit represents a traffic class.
> > So for 3 traffic classes at gate_mask of 0x7 is valid but this check
> > fails with 0x7 >= 3.
> 
> Additionally whatever check we put in place previously just ignored by
> the existing code, could break the existing user-space: we can't accept
> such change. 

I agree, and I would oppose erroring out.

I used to have this patch which simply masks off the excess bits,
calling netdev_warn() - which can be transformed into a warning netlink
extack - instead.
https://patchwork.kernel.org/project/netdevbpf/patch/20230130173145.475943-15-vladimir.oltean@nxp.com/

I didn't have a strong motivation for the patch, and I dropped it.
If Lai Peter Jun Ann can come with the motivation, we can go with that
approach.