[PATCH net] net: octeontx2-pf: Fix UB in shift operation

[PATCH net] net: octeontx2-pf: Fix UB in shift operation

[Sergey.V.Frolov]

From: "Sergey.V.Frolov" <Sergey.V.Frolov@kaspersky.com>

In function otx2_get_egress_burst_cfg, when the parameter `burst` is
255 and the max mantissa is 255 (0xFFULL), `burst_exp` is set to
`ilog2(255) - 1`, which equals 6.

This results in an unsigned wrap-around when calculating
`(1ULL << (*burst_exp - 7))`, since `*burst_exp - 7` becomes -1,
which makes the shift operand 0xFFFFFFFF. This value is greater than
the width of the left operand.

According to standard 6.5.7 p.3:
"The type of the result is that of the promoted left operand.
If the value of the right operand is negative or is greater than
or equal to the width of the promoted left operand, the behavior
is undefined."

Add a check to handle cases where `*burst_exp` is less than 7.

Signed-off-by: Sergey.V.Frolov <Sergey.V.Frolov@kaspersky.com>
---
 drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
index 40162b08014d..39bd73a214af 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
@@ -53,10 +53,15 @@ static void otx2_get_egress_burst_cfg(struct otx2_nic *nic, u32 burst,
 	if (burst) {
 		*burst_exp = ilog2(burst) ? ilog2(burst) - 1 : 0;
 		tmp = burst - rounddown_pow_of_two(burst);
-		if (burst < max_mantissa)
+		if (burst < max_mantissa) {
 			*burst_mantissa = tmp * 2;
-		else
-			*burst_mantissa = tmp / (1ULL << (*burst_exp - 7));
+		} else {
+			if (*burst_exp >= 7) {
+				*burst_mantissa = tmp / (1ULL << (*burst_exp - 7));
+			} else {
+				*burst_mantissa = tmp * (1ULL << (7 - *burst_exp));
+			}
+		}
 	} else {
 		*burst_exp = MAX_BURST_EXPONENT;
 		*burst_mantissa = max_mantissa;
-- 
2.34.1

Re: [PATCH net] net: octeontx2-pf: Fix UB in shift operation

[Ratheesh Kannoth]

On 2026-07-24 at 17:49:43, Sergey.V.Frolov@kaspersky.com (Sergey.V.Frolov@kaspersky.com) wrote:
> From: "Sergey.V.Frolov" <Sergey.V.Frolov@kaspersky.com>
>
> In function otx2_get_egress_burst_cfg, when the parameter `burst` is
> 255 and the max mantissa is 255 (0xFFULL), `burst_exp` is set to
> `ilog2(255) - 1`, which equals 6.
>
> This results in an unsigned wrap-around when calculating
> `(1ULL << (*burst_exp - 7))`, since `*burst_exp - 7` becomes -1,
> which makes the shift operand 0xFFFFFFFF. This value is greater than
> the width of the left operand.
>
> According to standard 6.5.7 p.3:
> "The type of the result is that of the promoted left operand.
> If the value of the right operand is negative or is greater than
> or equal to the width of the promoted left operand, the behavior
> is undefined."
>
> Add a check to handle cases where `*burst_exp` is less than 7.
>
> Signed-off-by: Sergey.V.Frolov <Sergey.V.Frolov@kaspersky.com>
> ---
>  drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> index 40162b08014d..39bd73a214af 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> @@ -53,10 +53,15 @@ static void otx2_get_egress_burst_cfg(struct otx2_nic *nic, u32 burst,
>  	if (burst) {
>  		*burst_exp = ilog2(burst) ? ilog2(burst) - 1 : 0;
>  		tmp = burst - rounddown_pow_of_two(burst);
> -		if (burst < max_mantissa)
> +		if (burst <= max_mantissa) {

Thanks.
can we handle 255 case with <= ? it looks simple to me.

Re: [PATCH net] net: octeontx2-pf: Fix UB in shift operation

[Sergey V. Frolov]

On Mon, 2026-07-27 at 08:57 +0530, Ratheesh Kannoth wrote:
> 
> > 
> > Signed-off-by: Sergey.V.Frolov <Sergey.V.Frolov@kaspersky.com>
> > ---
> >  drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c | 11
> > ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> > b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> > index 40162b08014d..39bd73a214af 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> > @@ -53,10 +53,15 @@ static void otx2_get_egress_burst_cfg(struct
> > otx2_nic *nic, u32 burst,
> >       if (burst) {
> >               *burst_exp = ilog2(burst) ? ilog2(burst) - 1 : 0;
> >               tmp = burst - rounddown_pow_of_two(burst);
> > -             if (burst < max_mantissa)
> > +             if (burst <= max_mantissa) {
> 
> Thanks.
> can we handle 255 case with <= ? it looks simple to me.

Thank you for reviewing the patch. I see that your simplification works
with the current value of max_mantissa (which is 255).

However, my concern is that this approach introduces an implicit
dependency on the fact that max_mantissa never being changed. If in the
future someone updates this constant (e.g. to support new hardware),
the shift operation could become invalid again, and the UB would
return. if max_mantissa becomes 127, then UB would reappear for burst
values in the range 128-255 (where burst_exp would be 6, making
*burst_exp - 7 = -1).

My original patch explicitly handles both cases (*burst_exp >= 7 and <
7), so it's robust regardless of max_mantissa value. I'd prefer to keep
the explicit check to avoid introducing a hidden dependency that might
affect future maintainability.

What do you think?

Re: [PATCH net] net: octeontx2-pf: Fix UB in shift operation

[Ratheesh Kannoth]

On 2026-07-27 at 17:16:24, Sergey V. Frolov (Sergey.V.Frolov@kaspersky.com) wrote:
> On Mon, 2026-07-27 at 08:57 +0530, Ratheesh Kannoth wrote:
> >
> > >
> > > Signed-off-by: Sergey.V.Frolov <Sergey.V.Frolov@kaspersky.com>
> > > ---
> > >  drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c | 11
> > > ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> > > b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> > > index 40162b08014d..39bd73a214af 100644
> > > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> > > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> > > @@ -53,10 +53,15 @@ static void otx2_get_egress_burst_cfg(struct
> > > otx2_nic *nic, u32 burst,
> > >       if (burst) {
> > >               *burst_exp = ilog2(burst) ? ilog2(burst) - 1 : 0;
> > >               tmp = burst - rounddown_pow_of_two(burst);
> > > -             if (burst < max_mantissa)
> > > +             if (burst <= max_mantissa) {
> >
> > Thanks.
> > can we handle 255 case with <= ? it looks simple to me.
>
> Thank you for reviewing the patch. I see that your simplification works
> with the current value of max_mantissa (which is 255).
>
> However, my concern is that this approach introduces an implicit
> dependency on the fact that max_mantissa never being changed. If in the
> future someone updates this constant (e.g. to support new hardware),
> the shift operation could become invalid again, and the UB would
> return. if max_mantissa becomes 127, then UB would reappear for burst
> values in the range 128-255 (where burst_exp would be 6, making
> *burst_exp - 7 = -1).
>
> My original patch explicitly handles both cases (*burst_exp >= 7 and <
> 7), so it's robust regardless of max_mantissa value. I'd prefer to keep
> the explicit check to avoid introducing a hidden dependency that might
> affect future maintainability.
>
> What do you think?

Thank you for the clarification.

I appreciate the robustness argument, but I'd like to provide some
hardware context that I think is relevant here.

On newer platforms (CN10K/CN20K), max_mantissa is 32767 — significantly
larger than the current OTX2/98xx value of 255. Given this trend, a
future reduction of max_mantissa below 256 would represent a major
architectural regression and is, in practice, highly unlikely.

It is also worth noting that in the proposed patch, the new else branch:

*burst_mantissa = tmp * (1ULL << (7 - *burst_exp));
is entirely unreachable dead code on CN10K/CN20K.

Rather than carrying dead code to guard against a hypothetical future
scenario, Could you consider following minimal approach:

1) Fix the off-by-one boundary condition by changing:
if (burst < max_mantissa)
to
if (burst <= max_mantissa)

2) Add a WARN_ON(*burst_exp < 7) before the else branch as an
explicit safeguard. This ensures that if max_mantissa ever changes
in a way that reintroduces this condition, it will be immediately
caught at runtime rather than silently triggering UB.

This keeps the fix minimal and correct for all current platforms, while
still providing a clear safety net against future regressions — without
adding unreachable code paths.

Re: [PATCH net] net: octeontx2-pf: Fix UB in shift operation

[Sergey V. Frolov]

> 1) Fix the off-by-one boundary condition by changing:
> if (burst < max_mantissa)
> to
> if (burst <= max_mantissa)
> 
> 2) Add a WARN_ON(*burst_exp < 7) before the else branch as an
> explicit safeguard. This ensures that if max_mantissa ever changes
> in a way that reintroduces this condition, it will be immediately
> caught at runtime rather than silently triggering UB.
> 
> This keeps the fix minimal and correct for all current platforms, while
> still providing a clear safety net against future regressions — without
> adding unreachable code paths.

Thank you for the detailed explanation.

I like the WARN_ON suggestion. Combined with changing
`if (burst < max_mantissa)` to `if (burst <= max_mantissa)`,
this looks safe and clean to me. I'm happy to go with your approach.

Acked-by: Sergey V. Frolov <Sergey.V.Frolov@kaspersky.com>

Thanks,
Sergey