* [bug report] net: dsa: mv88e6xxx: Fix out-of-bound access
@ 2024-08-23 12:46 Dan Carpenter
2024-08-23 14:40 ` Joseph Huang
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2024-08-23 12:46 UTC (permalink / raw)
To: Joseph Huang; +Cc: netdev
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] net: dsa: mv88e6xxx: Fix out-of-bound access
2024-08-23 12:46 [bug report] net: dsa: mv88e6xxx: Fix out-of-bound access Dan Carpenter
@ 2024-08-23 14:40 ` Joseph Huang
2024-08-23 16:58 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Joseph Huang @ 2024-08-23 14:40 UTC (permalink / raw)
To: Dan Carpenter, Joseph Huang; +Cc: netdev
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] net: dsa: mv88e6xxx: Fix out-of-bound access
2024-08-23 14:40 ` Joseph Huang
@ 2024-08-23 16:58 ` Dan Carpenter
2024-08-23 17:31 ` Joseph Huang
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2024-08-23 16:58 UTC (permalink / raw)
To: Joseph Huang; +Cc: Joseph Huang, netdev
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] net: dsa: mv88e6xxx: Fix out-of-bound access
2024-08-23 16:58 ` Dan Carpenter
@ 2024-08-23 17:31 ` Joseph Huang
0 siblings, 0 replies; 4+ messages in thread
From: Joseph Huang @ 2024-08-23 17:31 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Joseph Huang, netdev
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-08-23 17:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-23 12:46 [bug report] net: dsa: mv88e6xxx: Fix out-of-bound access Dan Carpenter
2024-08-23 14:40 ` Joseph Huang
2024-08-23 16:58 ` Dan Carpenter
2024-08-23 17:31 ` Joseph Huang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).