* [PATCH net 1/1] net: dsa: mv88e6xxx: Fix out-of-bound access
@ 2024-08-19 22:26 Joseph Huang
2024-08-19 22:58 ` Andrew Lunn
0 siblings, 1 reply; 5+ messages in thread
From: Joseph Huang @ 2024-08-19 22:26 UTC (permalink / raw)
To: netdev
Cc: Joseph Huang, Andrew Lunn, Florian Fainelli, Vladimir Oltean,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel
If an ATU violation was caused by a CPU Load operation, the SPID is 0xf,
which is larger than DSA_MAX_PORTS (the size of mv88e6xxx_chip.ports[]
array).
Fixes: 75c05a74e745 ("net: dsa: mv88e6xxx: Fix counting of ATU violations")
Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
---
drivers/net/dsa/mv88e6xxx/global1.h | 1 +
drivers/net/dsa/mv88e6xxx/global1_atu.c | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mv88e6xxx/global1.h b/drivers/net/dsa/mv88e6xxx/global1.h
index 3dbb7a1b8fe1..9676e2d42c9e 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.h
+++ b/drivers/net/dsa/mv88e6xxx/global1.h
@@ -162,6 +162,7 @@
#define MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC_AVB_NRL_PO 0x000d
#define MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC_DA_MGMT_PO 0x000e
#define MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC_PO 0x000f
+#define MV88E6XXX_G1_ATU_DATA_SPID_CPU 0x000f
/* Offset 0x0D: ATU MAC Address Register Bytes 0 & 1
* Offset 0x0E: ATU MAC Address Register Bytes 2 & 3
diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
index ce3b3690c3c0..b6f15ae22c20 100644
--- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
+++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
@@ -457,7 +457,8 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
trace_mv88e6xxx_atu_full_violation(chip->dev, spid,
entry.portvec, entry.mac,
fid);
- chip->ports[spid].atu_full_violation++;
+ if (spid != MV88E6XXX_G1_ATU_DATA_SPID_CPU)
+ chip->ports[spid].atu_full_violation++;
}
return IRQ_HANDLED;
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net 1/1] net: dsa: mv88e6xxx: Fix out-of-bound access
2024-08-19 22:26 [PATCH net 1/1] net: dsa: mv88e6xxx: Fix out-of-bound access Joseph Huang
@ 2024-08-19 22:58 ` Andrew Lunn
2024-08-20 18:32 ` Simon Horman
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Lunn @ 2024-08-19 22:58 UTC (permalink / raw)
To: Joseph Huang
Cc: netdev, Florian Fainelli, Vladimir Oltean, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-kernel
On Mon, Aug 19, 2024 at 06:26:40PM -0400, Joseph Huang wrote:
> If an ATU violation was caused by a CPU Load operation, the SPID is 0xf,
> which is larger than DSA_MAX_PORTS (the size of mv88e6xxx_chip.ports[]
> array).
The 6390X datasheet says "IF SPID = 0x1f the source of the violation
was the CPU's registers interface."
> +#define MV88E6XXX_G1_ATU_DATA_SPID_CPU 0x000f
So it seems to depend on the family.
>
> /* Offset 0x0D: ATU MAC Address Register Bytes 0 & 1
> * Offset 0x0E: ATU MAC Address Register Bytes 2 & 3
> diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
> index ce3b3690c3c0..b6f15ae22c20 100644
> --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
> +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
> @@ -457,7 +457,8 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
> trace_mv88e6xxx_atu_full_violation(chip->dev, spid,
> entry.portvec, entry.mac,
> fid);
> - chip->ports[spid].atu_full_violation++;
> + if (spid != MV88E6XXX_G1_ATU_DATA_SPID_CPU)
> + chip->ports[spid].atu_full_violation++;
So i think it would be better to do something like:
if (spid < ARRAY_SIZE(chip->ports))
chip->ports[spid].atu_full_violation++;
Andrew
---
pw-bot: cr
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net 1/1] net: dsa: mv88e6xxx: Fix out-of-bound access
2024-08-19 22:58 ` Andrew Lunn
@ 2024-08-20 18:32 ` Simon Horman
2024-08-20 19:21 ` Joseph Huang
0 siblings, 1 reply; 5+ messages in thread
From: Simon Horman @ 2024-08-20 18:32 UTC (permalink / raw)
To: Andrew Lunn
Cc: Joseph Huang, netdev, Florian Fainelli, Vladimir Oltean,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel
On Tue, Aug 20, 2024 at 12:58:05AM +0200, Andrew Lunn wrote:
> On Mon, Aug 19, 2024 at 06:26:40PM -0400, Joseph Huang wrote:
> > If an ATU violation was caused by a CPU Load operation, the SPID is 0xf,
> > which is larger than DSA_MAX_PORTS (the size of mv88e6xxx_chip.ports[]
> > array).
>
> The 6390X datasheet says "IF SPID = 0x1f the source of the violation
> was the CPU's registers interface."
>
> > +#define MV88E6XXX_G1_ATU_DATA_SPID_CPU 0x000f
>
> So it seems to depend on the family.
>
> >
> > /* Offset 0x0D: ATU MAC Address Register Bytes 0 & 1
> > * Offset 0x0E: ATU MAC Address Register Bytes 2 & 3
> > diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
> > index ce3b3690c3c0..b6f15ae22c20 100644
> > --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
> > +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
> > @@ -457,7 +457,8 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
> > trace_mv88e6xxx_atu_full_violation(chip->dev, spid,
> > entry.portvec, entry.mac,
> > fid);
> > - chip->ports[spid].atu_full_violation++;
> > + if (spid != MV88E6XXX_G1_ATU_DATA_SPID_CPU)
> > + chip->ports[spid].atu_full_violation++;
>
> So i think it would be better to do something like:
>
> if (spid < ARRAY_SIZE(chip->ports))
> chip->ports[spid].atu_full_violation++;
Hi Joseph,
I am curious to know if bounds checking should also
be added to other accesses to chip->ports[spid] within this function.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net 1/1] net: dsa: mv88e6xxx: Fix out-of-bound access
2024-08-20 18:32 ` Simon Horman
@ 2024-08-20 19:21 ` Joseph Huang
2024-08-21 14:13 ` Simon Horman
0 siblings, 1 reply; 5+ messages in thread
From: Joseph Huang @ 2024-08-20 19:21 UTC (permalink / raw)
To: Simon Horman, Andrew Lunn
Cc: Joseph Huang, netdev, Florian Fainelli, Vladimir Oltean,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel
On 8/20/2024 2:32 PM, Simon Horman wrote:
> On Tue, Aug 20, 2024 at 12:58:05AM +0200, Andrew Lunn wrote:
>> On Mon, Aug 19, 2024 at 06:26:40PM -0400, Joseph Huang wrote:
>> > If an ATU violation was caused by a CPU Load operation, the SPID is 0xf,
>> > which is larger than DSA_MAX_PORTS (the size of mv88e6xxx_chip.ports[]
>> > array).
>>
>> The 6390X datasheet says "IF SPID = 0x1f the source of the violation
>> was the CPU's registers interface."
>>
>> > +#define MV88E6XXX_G1_ATU_DATA_SPID_CPU 0x000f
>>
>> So it seems to depend on the family.
>>
>> >
>> > /* Offset 0x0D: ATU MAC Address Register Bytes 0 & 1
>> > * Offset 0x0E: ATU MAC Address Register Bytes 2 & 3
>> > diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
>> > index ce3b3690c3c0..b6f15ae22c20 100644
>> > --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
>> > +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
>> > @@ -457,7 +457,8 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
>> > trace_mv88e6xxx_atu_full_violation(chip->dev, spid,
>> > entry.portvec, entry.mac,
>> > fid);
>> > - chip->ports[spid].atu_full_violation++;
>> > + if (spid != MV88E6XXX_G1_ATU_DATA_SPID_CPU)
>> > + chip->ports[spid].atu_full_violation++;
>>
>> So i think it would be better to do something like:
>>
>> if (spid < ARRAY_SIZE(chip->ports))
>> chip->ports[spid].atu_full_violation++;
>
> Hi Joseph,
>
> I am curious to know if bounds checking should also
> be added to other accesses to chip->ports[spid] within this function.
>
Hi Simon,
From the spec it is unclear to me whether the Load operation could
actually cause other exceptions. I was only able to reproduce and verify
the full violation, and that's why I only included that one in the patch.
I guess we could proactively include the fix for other exceptions as
well, but without a way to verify them, they could be just dead code and
never be exercised. Perhaps people who are more familiar with the chip
than me could chime in. I'm fine either way.
Thanks,
Joseph
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net 1/1] net: dsa: mv88e6xxx: Fix out-of-bound access
2024-08-20 19:21 ` Joseph Huang
@ 2024-08-21 14:13 ` Simon Horman
0 siblings, 0 replies; 5+ messages in thread
From: Simon Horman @ 2024-08-21 14:13 UTC (permalink / raw)
To: Joseph Huang
Cc: Andrew Lunn, Joseph Huang, netdev, Florian Fainelli,
Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, linux-kernel
On Tue, Aug 20, 2024 at 03:21:57PM -0400, Joseph Huang wrote:
> On 8/20/2024 2:32 PM, Simon Horman wrote:
> > On Tue, Aug 20, 2024 at 12:58:05AM +0200, Andrew Lunn wrote:
> > > On Mon, Aug 19, 2024 at 06:26:40PM -0400, Joseph Huang wrote:
> > > > If an ATU violation was caused by a CPU Load operation, the SPID is 0xf,
> > > > which is larger than DSA_MAX_PORTS (the size of mv88e6xxx_chip.ports[]
> > > > array).
> > >
> > > The 6390X datasheet says "IF SPID = 0x1f the source of the violation
> > > was the CPU's registers interface."
> > >
> > > > +#define MV88E6XXX_G1_ATU_DATA_SPID_CPU 0x000f
> > >
> > > So it seems to depend on the family.
> > >
> > > > > /* Offset 0x0D: ATU MAC Address Register Bytes 0 & 1
> > > > * Offset 0x0E: ATU MAC Address Register Bytes 2 & 3
> > > > diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
> > > > index ce3b3690c3c0..b6f15ae22c20 100644
> > > > --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
> > > > +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
> > > > @@ -457,7 +457,8 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
> > > > trace_mv88e6xxx_atu_full_violation(chip->dev, spid,
> > > > entry.portvec, entry.mac,
> > > > fid);
> > > > - chip->ports[spid].atu_full_violation++;
> > > > + if (spid != MV88E6XXX_G1_ATU_DATA_SPID_CPU)
> > > > + chip->ports[spid].atu_full_violation++;
> > >
> > > So i think it would be better to do something like:
> > >
> > > if (spid < ARRAY_SIZE(chip->ports))
> > > chip->ports[spid].atu_full_violation++;
> >
> > Hi Joseph,
> >
> > I am curious to know if bounds checking should also
> > be added to other accesses to chip->ports[spid] within this function.
> >
>
> Hi Simon,
>
> From the spec it is unclear to me whether the Load operation could actually
> cause other exceptions. I was only able to reproduce and verify the full
> violation, and that's why I only included that one in the patch.
>
> I guess we could proactively include the fix for other exceptions as well,
> but without a way to verify them, they could be just dead code and never be
> exercised. Perhaps people who are more familiar with the chip than me could
> chime in. I'm fine either way.
Thanks Joseph,
From my PoV it would be nice to add the checks unless we can be sure they
are not needed. But I do not feel strongly about this.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-08-21 14:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-19 22:26 [PATCH net 1/1] net: dsa: mv88e6xxx: Fix out-of-bound access Joseph Huang
2024-08-19 22:58 ` Andrew Lunn
2024-08-20 18:32 ` Simon Horman
2024-08-20 19:21 ` Joseph Huang
2024-08-21 14:13 ` Simon Horman
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).