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

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: <4b004e58-60ca-4042-8f42-3e36e1c493e5@stanley.mountain>
Date: Fri, 23 Aug 2024 19:58:30 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Joseph Huang <joseph.huang.2024@...il.com>
Cc: Joseph Huang <Joseph.Huang@...min.com>, netdev@...r.kernel.org
Subject: Re: [bug report] net: dsa: mv88e6xxx: Fix out-of-bound access

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