Re: [bug report] net: dsa: mv88e6xxx: Fix out-of-bound access

[Joseph Huang <joseph.huang.2024@gmail.com>]

On 8/23/2024 12:58 PM, Dan Carpenter wrote:
> On Fri, Aug 23, 2024 at 10:40:52AM -0400, Joseph Huang wrote:
>>
>> Hi Dan,
>>
>> I had a similar discussion with Simon on this issue (see https://lore.kernel.org/lkml/5da4cc4d-2e68-424c-8d91-299d3ccb6dc8@gmail.com/).
>> The spid in question here should point to a physical port to indicate which
>> port caused the exception (DSA_MAX_PORTS is defined to cover the maximum
>> number of physical ports any DSA device can possibly have). Only when the
>> exception is caused by a CPU Load operation will the spid be a hardcoded
>> value which is greater than the array size. The ATU Full exception is the
>> only one (that I know of) that could be caused by a CPU Load operation,
>> that's why the check is only added/needed for that particular exception
>> case.
>>
> 
> That doesn't really answer the question if multiple flags can be set at once
> but presumably not.  The ->ports array has DSA_MAX_PORTS (12) elements.
> I used Smatch to see where ->state is set to see where it can be out of bounds.
> 
> $ smdb.py where mv88e6xxx_atu_entry state
> drivers/net/dsa/mv88e6xxx/devlink.c | mv88e6xxx_region_atu_snapshot_fid | (struct mv88e6xxx_atu_entry)->state | 0
> drivers/net/dsa/mv88e6xxx/global1_atu.c | mv88e6xxx_g1_atu_data_read     | (struct mv88e6xxx_atu_entry)->state | 0-15
> drivers/net/dsa/mv88e6xxx/global1_atu.c | mv88e6xxx_g1_atu_flush         | (struct mv88e6xxx_atu_entry)->state | 0
> drivers/net/dsa/mv88e6xxx/global1_atu.c | mv88e6xxx_g1_atu_move          | (struct mv88e6xxx_atu_entry)->state | 0,15
> drivers/net/dsa/mv88e6xxx/chip.c | mv88e6xxx_port_db_load_purge   | (struct mv88e6xxx_atu_entry)->state | 0,4,7-8,14
> drivers/net/dsa/mv88e6xxx/chip.c | mv88e6xxx_port_db_dump_fid     | (struct mv88e6xxx_atu_entry)->state | 0
> 
> mv88e6xxx_g1_atu_move() is what you fixed:
> 	entry.state = 0xf; /* Full EntryState means Move */
> 
> mv88e6xxx_g1_atu_data_read() does "entry->state = val & 0xf;" so that's why
> Smatch says it's 0-15.  The actual "val" comes from mv88e6xxx_g1_atu_data_write()
> and is complicated.
> 
> mv88e6xxx_port_db_load_purge() sets ->state to MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC (14).
> I would still be concerned about that.
> 
> regards,
> dan carpenter
> 

Hi Dan,

The field (->state) could mean either ATU Entry State or SPID (Source 
Port ID), two unrelated attributes, depending on the context. In all of 
the functions above, that field means ATU Entry State. The field means 
SPID only in the exception handlers' context. The value (ATU Entry 
State) being written to in the above functions does not correspond to 
the value being read back (SPID) in the exception handlers.

HTH
Joseph