Bug 263896 - Interger overflow in buffer size calculation in sys/dev/e1000/if_em.c
Summary: Interger overflow in buffer size calculation in sys/dev/e1000/if_em.c
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: Unspecified
Hardware: Any Any
: --- Affects Some People
Assignee: Kevin Bowling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-05-10 13:13 UTC by hannula
Modified: 2022-05-15 17:05 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description hannula 2022-05-10 13:13:36 UTC
In sys/dev/e1000/if_em.c the RX buffer is defined as:
u16 rx_buffer_size

so max value is 65535.

Later it is used to calculate the buffer water levels:
rx_buffer_size = (pba & 0xffff) << 10;
hw->fc.high_water = rx_buffer_size - roundup2(hw->mac.max_frame_size, 1024);

This is ok for example for 36KB when pba is 0x36 fc.high_water is calculated as 36864-2048=34816.

But for I350 when only 2 ports are used PBA size can be set as 72KB (see datasheet RXPbsize or e1000_rxpbs_adjust_82580 function in e1000_82575.c).
In this case calculating the rx_buffer_size overflows as 0x0048 << 10 = 73728 or 0x12000 pushed into u16. It is then set as 0x2000 or 8192.

Tested on my dual port I350 vs quad port I350:
On quad port the PBA size is 36KB and is calculated correctly 36864-2048=34816:
dev.igb.0.fc_low_water: 34800
dev.igb.0.fc_high_water: 34816

On dual port the PBA size is 72KB and totally wrong values are set corresponding to 8192-2048=6144:
dev.igb.0.fc_low_water: 6128
dev.igb.0.fc_high_water: 6144

One can verify what PBA is set in hardware with enabling verbose boot:
kernel: em_reset: pba=36K
kernel: em_reset: pba=72K
Comment 1 Kevin Bowling freebsd_committer freebsd_triage 2022-05-10 16:25:31 UTC
Thanks for the detailed report.  Can you try the one line change in https://reviews.freebsd.org/D35167
Comment 2 hannula 2022-05-12 09:36:50 UTC
Thanks for the quick fix!

When using u32 instead of u16 the values are what is expected:
dev.igb.0.fc_low_water: 71664
dev.igb.0.fc_high_water: 71680
Comment 3 commit-hook freebsd_committer freebsd_triage 2022-05-12 15:45:11 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=6987c47569b377f4b6eba9966afdedfb1b39fca8

commit 6987c47569b377f4b6eba9966afdedfb1b39fca8
Author:     Kevin Bowling <kbowling@FreeBSD.org>
AuthorDate: 2022-05-12 15:38:09 +0000
Commit:     Kevin Bowling <kbowling@FreeBSD.org>
CommitDate: 2022-05-12 15:43:55 +0000

    e1000: Increase rx_buffer_size to 32b

    Extend the size of the local rx_buffer_size variable to account for
    larger buffer sizes possible on 82580, i350 chips.

    From i350 datasheet, 6.2.10 Initialization Control 4 (LAN Base Address
    + Offset 0x13):
    When 4 ports are enabled maximum buffer size is 36 KB. When 2 ports are
    enabled maximum buffer size is 72 KB. When only a single port is
    enabled maximum buffer size is 144 KB.

    and 8.3:
    The overall available internal buffer size in the I350 for all ports is
    144 KB for receive buffers and 80 KB for transmit Buffers. Disabled
    ports memory can be shared between active ports and sharing can be
    asymmetric. The default buffer size for each port is loaded from the
    EEPROM on initialization.

    From the reporter:
    But for I350 when only 2 ports are used PBA size can be set as 72KB
    (see datasheet RXPbsize or e1000_rxpbs_adjust_82580 function in
    e1000_82575.c). In this case calculating the rx_buffer_size overflows
    as 0x0048 << 10 = 73728 or 0x12000 pushed into u16. It is then set as
    0x2000 or 8192.

    PR:             263896
    Reported by:    hannula@gmail.com
    Tested by:      hannula@gmail.com
    Approved by:    markj
    MFC after:      3 days
    Differential Revision:  https://reviews.freebsd.org/D35167

 sys/dev/e1000/if_em.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 4 commit-hook freebsd_committer freebsd_triage 2022-05-15 17:04:25 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=3f8306cf8e2d817f3c50ec2ecdfa52a71f2515ae

commit 3f8306cf8e2d817f3c50ec2ecdfa52a71f2515ae
Author:     Kevin Bowling <kbowling@FreeBSD.org>
AuthorDate: 2022-05-12 15:38:09 +0000
Commit:     Kevin Bowling <kbowling@FreeBSD.org>
CommitDate: 2022-05-15 17:04:00 +0000

    e1000: Increase rx_buffer_size to 32b

    Extend the size of the local rx_buffer_size variable to account for
    larger buffer sizes possible on 82580, i350 chips.

    From i350 datasheet, 6.2.10 Initialization Control 4 (LAN Base Address
    + Offset 0x13):
    When 4 ports are enabled maximum buffer size is 36 KB. When 2 ports are
    enabled maximum buffer size is 72 KB. When only a single port is
    enabled maximum buffer size is 144 KB.

    and 8.3:
    The overall available internal buffer size in the I350 for all ports is
    144 KB for receive buffers and 80 KB for transmit Buffers. Disabled
    ports memory can be shared between active ports and sharing can be
    asymmetric. The default buffer size for each port is loaded from the
    EEPROM on initialization.

    From the reporter:
    But for I350 when only 2 ports are used PBA size can be set as 72KB
    (see datasheet RXPbsize or e1000_rxpbs_adjust_82580 function in
    e1000_82575.c). In this case calculating the rx_buffer_size overflows
    as 0x0048 << 10 = 73728 or 0x12000 pushed into u16. It is then set as
    0x2000 or 8192.

    PR:             263896
    Reported by:    hannula@gmail.com
    Tested by:      hannula@gmail.com
    Approved by:    markj
    Differential Revision:  https://reviews.freebsd.org/D35167

    (cherry picked from commit 6987c47569b377f4b6eba9966afdedfb1b39fca8)

 sys/dev/e1000/if_em.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 5 commit-hook freebsd_committer freebsd_triage 2022-05-15 17:05:26 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=f4262a065dcf32169d2e8b5434a5030bf60f1ae3

commit f4262a065dcf32169d2e8b5434a5030bf60f1ae3
Author:     Kevin Bowling <kbowling@FreeBSD.org>
AuthorDate: 2022-05-12 15:38:09 +0000
Commit:     Kevin Bowling <kbowling@FreeBSD.org>
CommitDate: 2022-05-15 17:04:38 +0000

    e1000: Increase rx_buffer_size to 32b

    Extend the size of the local rx_buffer_size variable to account for
    larger buffer sizes possible on 82580, i350 chips.

    From i350 datasheet, 6.2.10 Initialization Control 4 (LAN Base Address
    + Offset 0x13):
    When 4 ports are enabled maximum buffer size is 36 KB. When 2 ports are
    enabled maximum buffer size is 72 KB. When only a single port is
    enabled maximum buffer size is 144 KB.

    and 8.3:
    The overall available internal buffer size in the I350 for all ports is
    144 KB for receive buffers and 80 KB for transmit Buffers. Disabled
    ports memory can be shared between active ports and sharing can be
    asymmetric. The default buffer size for each port is loaded from the
    EEPROM on initialization.

    From the reporter:
    But for I350 when only 2 ports are used PBA size can be set as 72KB
    (see datasheet RXPbsize or e1000_rxpbs_adjust_82580 function in
    e1000_82575.c). In this case calculating the rx_buffer_size overflows
    as 0x0048 << 10 = 73728 or 0x12000 pushed into u16. It is then set as
    0x2000 or 8192.

    PR:             263896
    Reported by:    hannula@gmail.com
    Tested by:      hannula@gmail.com
    Approved by:    markj
    Differential Revision:  https://reviews.freebsd.org/D35167

    (cherry picked from commit 6987c47569b377f4b6eba9966afdedfb1b39fca8)

 sys/dev/e1000/if_em.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)