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: <0b6376c2-bd04-4090-a3bf-b58587bbe307@gmail.com>
Date: Fri, 23 Aug 2024 10:40:52 -0400
From: Joseph Huang <joseph.huang.2024@...il.com>
To: Dan Carpenter <dan.carpenter@...aro.org>,
 Joseph Huang <Joseph.Huang@...min.com>
Cc: netdev@...r.kernel.org
Subject: Re: [bug report] net: dsa: mv88e6xxx: Fix out-of-bound access

On 8/23/2024 8:46 AM, Dan Carpenter wrote:
> 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
> 

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.

Thanks,
Joseph