lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5da4cc4d-2e68-424c-8d91-299d3ccb6dc8@gmail.com>
Date: Tue, 20 Aug 2024 15:21:57 -0400
From: Joseph Huang <joseph.huang.2024@...il.com>
To: Simon Horman <horms@...nel.org>, Andrew Lunn <andrew@...n.ch>
Cc: Joseph Huang <Joseph.Huang@...min.com>, netdev@...r.kernel.org,
 Florian Fainelli <f.fainelli@...il.com>, Vladimir Oltean
 <olteanv@...il.com>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 1/1] net: dsa: mv88e6xxx: Fix out-of-bound access

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ