netdev - [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-next>] [day] [month] [year] [list]

Message-ID: <d9d8c03e-a3d9-4480-af99-c509ed9b8d8d@stanley.mountain>
Date: Fri, 23 Aug 2024 15:46:30 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Joseph Huang <Joseph.Huang@...min.com>
Cc: netdev@...r.kernel.org
Subject: [bug report] net: dsa: mv88e6xxx: Fix out-of-bound access

Hello Joseph Huang,

Commit 528876d867a2 ("net: dsa: mv88e6xxx: Fix out-of-bound access")
from Aug 19, 2024 (linux-next), leads to the following Smatch static
checker warning:

	drivers/net/dsa/mv88e6xxx/global1_atu.c:460 mv88e6xxx_g1_atu_prob_irq_thread_fn()
	error: testing array offset 'spid' after use.

drivers/net/dsa/mv88e6xxx/global1_atu.c
    402 static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
    403 {
    404         struct mv88e6xxx_chip *chip = dev_id;
    405         struct mv88e6xxx_atu_entry entry;
    406         int err, spid;
    407         u16 val, fid;
    408 
    409         mv88e6xxx_reg_lock(chip);
    410 
    411         err = mv88e6xxx_g1_read_atu_violation(chip);
    412         if (err)
    413                 goto out_unlock;
    414 
    415         err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_ATU_OP, &val);
    416         if (err)
    417                 goto out_unlock;
    418 
    419         err = mv88e6xxx_g1_atu_fid_read(chip, &fid);
    420         if (err)
    421                 goto out_unlock;
    422 
    423         err = mv88e6xxx_g1_atu_data_read(chip, &entry);
    424         if (err)
    425                 goto out_unlock;
    426 
    427         err = mv88e6xxx_g1_atu_mac_read(chip, &entry);
    428         if (err)
    429                 goto out_unlock;
    430 
    431         mv88e6xxx_reg_unlock(chip);
    432 
    433         spid = entry.state;
    434 
    435         if (val & MV88E6XXX_G1_ATU_OP_MEMBER_VIOLATION) {
    436                 trace_mv88e6xxx_atu_member_violation(chip->dev, spid,
    437                                                      entry.portvec, entry.mac,
    438                                                      fid);
    439                 chip->ports[spid].atu_member_violation++;
                                    ^^^^

The commit adds a bounds check later if the MV88E6XXX_G1_ATU_OP_FULL_VIOLATION
flag is set but it doesn't add it here where MV88E6XXX_G1_ATU_OP_MEMBER_VIOLATION
is set.  Can only one type of violation flag be set at a time?

    440         }
    441 
    442         if (val & MV88E6XXX_G1_ATU_OP_MISS_VIOLATION) {
    443                 trace_mv88e6xxx_atu_miss_violation(chip->dev, spid,
    444                                                    entry.portvec, entry.mac,
    445                                                    fid);
    446                 chip->ports[spid].atu_miss_violation++;
                                    ^^^^

    447 
    448                 if (fid != MV88E6XXX_FID_STANDALONE && chip->ports[spid].mab) {
                                                                     ^^^^^^^^^^^

    449                         err = mv88e6xxx_handle_miss_violation(chip, spid,
    450                                                               &entry, fid);
    451                         if (err)
    452                                 goto out;
    453                 }
    454         }
    455 
    456         if (val & MV88E6XXX_G1_ATU_OP_FULL_VIOLATION) {
    457                 trace_mv88e6xxx_atu_full_violation(chip->dev, spid,
    458                                                    entry.portvec, entry.mac,
    459                                                    fid);
--> 460                 if (spid < ARRAY_SIZE(chip->ports))
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is the new check.

    461                         chip->ports[spid].atu_full_violation++;
    462         }
    463 
    464         return IRQ_HANDLED;
    465 
    466 out_unlock:
    467         mv88e6xxx_reg_unlock(chip);
    468 
    469 out:
    470         dev_err(chip->dev, "ATU problem: error %d while handling interrupt\n",
    471                 err);
    472         return IRQ_HANDLED;
    473 }

regards,
dan carpenter