| 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
| ||
|
Message-ID: <aRuOh-O99Xwo1nG0@horms.kernel.org>
Date: Mon, 17 Nov 2025 21:07:19 +0000
From: Simon Horman <horms@...nel.org>
To: Parvathi Pudi <parvathi@...thit.com>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, danishanwar@...com,
rogerq@...nel.org, pmohan@...thit.com, basharath@...thit.com,
afd@...com, linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, alok.a.tiwari@...cle.com,
pratheesh@...com, j-rameshbabu@...com, vigneshr@...com,
praneeth@...com, srk@...com, rogerq@...com, krishna@...thit.com,
mohan@...thit.com
Subject: Re: [PATCH net-next v5 1/3] net: ti: icssm-prueth: Adds helper
functions to configure and maintain FDB
On Thu, Nov 13, 2025 at 03:40:21PM +0530, Parvathi Pudi wrote:
...
> +static u16 icssm_prueth_sw_fdb_find_open_slot(struct fdb_tbl *fdb_tbl)
> +{
> + u8 flags;
> + u16 i;
> +
> + for (i = 0; i < FDB_MAC_TBL_MAX_ENTRIES; i++) {
> + flags = readb(&fdb_tbl->mac_tbl_a->mac_tbl_entry[i].flags);
> + if (!(flags & FLAG_ACTIVE))
> + break;
Hi Parvathi, all,
If the condition above is never met....
> + }
> +
> + return i;
Then FDB_MAC_TBL_MAX_ENTRIES will be returned here.
Which is written to bkt_info->bucket_idx by
icssm_prueth_sw_insert_fdb_entry()...
> +}
> +
> +static int
> +icssm_prueth_sw_find_fdb_insert(struct fdb_tbl *fdb, struct prueth *prueth,
> + struct fdb_index_tbl_entry __iomem *bkt_info,
> + const u8 *mac, const u8 port)
> +{
> + struct fdb_mac_tbl_array __iomem *mac_tbl = fdb->mac_tbl_a;
> + struct fdb_mac_tbl_entry __iomem *e;
> + u8 mac_from_hw[ETH_ALEN];
> + u16 bucket_entries;
> + u16 mac_tbl_idx;
> + int i, ret;
> + s8 cmp;
> +
> + mac_tbl_idx = readw(&bkt_info->bucket_idx);
That value will be read here...
> + bucket_entries = readw(&bkt_info->bucket_entries);
> +
> + for (i = 0; i < bucket_entries; i++, mac_tbl_idx++) {
> + e = &mac_tbl->mac_tbl_entry[mac_tbl_idx];
And used here, without any bounds checking.
But if mac_tbl_idx is FDB_MAC_TBL_MAX_ENTRIES then
the access to mac_tble_entry will overflow, as
it only has FDB_MAC_TBL_MAX_ENTRIES elements.
This, and most of my review points for this patch set were
flagged by Claude Code with https://github.com/masoncl/review-prompts/
...
> +static int icssm_prueth_sw_insert_fdb_entry(struct prueth_emac *emac,
> + const u8 *mac, u8 is_static)
> +{
> + struct fdb_index_tbl_entry __iomem *bucket_info;
> + struct fdb_mac_tbl_entry __iomem *mac_info;
> + struct prueth *prueth = emac->prueth;
> + struct prueth_emac *other_emac;
> + enum prueth_port other_port_id;
> + u8 hash_val, mac_tbl_idx;
> + struct fdb_tbl *fdb;
> + u8 flags;
> + u16 val;
> + s16 ret;
> + int err;
> +
> + fdb = prueth->fdb_tbl;
> + other_port_id = (emac->port_id == PRUETH_PORT_MII0) ?
> + PRUETH_PORT_MII1 : PRUETH_PORT_MII0;
> +
> + other_emac = prueth->emac[other_port_id - 1];
> +
> + if (fdb->total_entries == FDB_MAC_TBL_MAX_ENTRIES)
> + return -ENOMEM;
Access to total_entries outside of icssm_prueth_sw_fdb_spin_lock.
Seems racy here. Likewise for the similar check in
icssm_prueth_sw_delete_fdb_entry().
Flagged by Claude Code with https://github.com/masoncl/review-prompts/
> +
> + if (ether_addr_equal(mac, emac->mac_addr) ||
> + ether_addr_equal(mac, other_emac->mac_addr)) {
> + /* Don't insert fdb of own mac addr */
> + return -EINVAL;
> + }
> +
> + /* Get the bucket that the mac belongs to */
> + hash_val = icssm_prueth_sw_fdb_hash(mac);
> + bucket_info = FDB_IDX_TBL_ENTRY(hash_val);
> +
> + if (!readw(&bucket_info->bucket_entries)) {
> + mac_tbl_idx = icssm_prueth_sw_fdb_find_open_slot(fdb);
> + writew(mac_tbl_idx, &bucket_info->bucket_idx);
> + }
> +
> + ret = icssm_prueth_sw_find_fdb_insert(fdb, prueth, bucket_info, mac,
> + emac->port_id - 1);
> + if (ret < 0)
> + /* mac is already in fdb table */
> + return 0;
> +
> + mac_tbl_idx = ret;
> +
> + err = icssm_prueth_sw_fdb_spin_lock(fdb);
> + if (err) {
> + dev_err(prueth->dev, "PRU lock timeout %d\n", ret);
> + return err;
> + }
> +
> + mac_info = icssm_prueth_sw_find_free_mac(prueth, bucket_info,
> + mac_tbl_idx, NULL,
> + mac);
> + if (!mac_info) {
> + /* Should not happen */
> + dev_warn(prueth->dev, "OUT of FDB MEM\n");
This appears to leak icssm_prueth_sw_fdb_spin_lock.
Also flagged by Claude Code with https://github.com/masoncl/review-prompts/
> + return -ENOMEM;
> + }
> +
> + memcpy_toio(mac_info->mac, mac, ETH_ALEN);
> + writew(0, &mac_info->age);
> + writeb(emac->port_id - 1, &mac_info->port);
> +
> + flags = readb(&mac_info->flags);
> + if (is_static)
> + flags |= FLAG_IS_STATIC;
> + else
> + flags &= ~FLAG_IS_STATIC;
> +
> + /* bit 1 - active */
> + flags |= FLAG_ACTIVE;
> + writeb(flags, &mac_info->flags);
> +
> + val = readw(&bucket_info->bucket_entries);
> + val++;
> + writew(val, &bucket_info->bucket_entries);
> +
> + fdb->total_entries++;
> +
> + icssm_prueth_sw_fdb_spin_unlock(fdb);
> +
> + dev_dbg(prueth->dev, "added fdb: %pM port=%d total_entries=%u\n",
> + mac, emac->port_id, fdb->total_entries);
Less important, but I think the value of total_entries could
be out of date as it's accessed outside of the lock.
Perhaps it would be good to stash the value into a local variable while
the lock is still held?
Likewise in icssm_prueth_sw_delete_fdb_entry().
> +
> + return 0;
> +}
> +
> +static int icssm_prueth_sw_delete_fdb_entry(struct prueth_emac *emac,
> + const u8 *mac, u8 is_static)
> +{
> + struct fdb_index_tbl_entry __iomem *bucket_info;
> + struct fdb_mac_tbl_entry __iomem *mac_info;
> + struct fdb_mac_tbl_array __iomem *mt;
> + u8 hash_val, mac_tbl_idx;
> + u16 idx, entries, val;
> + struct prueth *prueth;
> + s16 ret, left, right;
> + struct fdb_tbl *fdb;
> + u8 flags;
> + int err;
> +
> + prueth = emac->prueth;
> + fdb = prueth->fdb_tbl;
> + mt = fdb->mac_tbl_a;
> +
> + if (fdb->total_entries == 0)
> + return 0;
> +
> + /* Get the bucket that the mac belongs to */
> + hash_val = icssm_prueth_sw_fdb_hash(mac);
> + bucket_info = FDB_IDX_TBL_ENTRY(hash_val);
> +
> + ret = icssm_prueth_sw_fdb_search(mt, bucket_info, mac);
> + if (ret < 0)
> + return ret;
> +
> + mac_tbl_idx = ret;
> + mac_info = FDB_MAC_TBL_ENTRY(mac_tbl_idx);
> +
> + err = icssm_prueth_sw_fdb_spin_lock(fdb);
> + if (err) {
> + dev_err(prueth->dev, "PRU lock timeout %d\n", ret);
> + return ret;
> + }
> +
> + /* Shift all elements in bucket to the left. No need to
> + * update index table since only shifting within bucket.
> + */
> + left = mac_tbl_idx;
> + idx = readw(&bucket_info->bucket_idx);
> + entries = readw(&bucket_info->bucket_entries);
> + right = idx + entries - 1;
> + icssm_prueth_sw_fdb_move_range_left(prueth, left, right);
> +
> + /* Remove end of bucket from table */
> + mac_info = FDB_MAC_TBL_ENTRY(right);
> + flags = readb(&mac_info->flags);
> + /* active = 0 */
> + flags &= ~FLAG_ACTIVE;
> + writeb(flags, &mac_info->flags);
> + val = readw(&bucket_info->bucket_entries);
> + val--;
> + writew(val, &bucket_info->bucket_entries);
> + fdb->total_entries--;
> +
> + icssm_prueth_sw_fdb_spin_unlock(fdb);
> +
> + dev_dbg(prueth->dev, "del fdb: %pM total_entries=%u\n",
> + mac, fdb->total_entries);
> +
> + return 0;
> +}
...
Powered by blists - more mailing lists