When rx_buffer_len is not aligned by 1024, igbvf sets the aligned size
to SRRCTL while the buffer is allocated with the unaligned size. This
allows the device to write more data than rx_buffer_len, resulting in
memory corruption. Align rx_buffer_len itself so that the buffer will
be allocated with the aligned size.
The condition to split RX packet header, which uses rx_buffer_len, is
also modified so that it doesn't change the behavior for the same
actual (unaligned) packet size. Actually the new condition is not
identical with the old one as it will no longer request splitting when
the actual packet size is exactly 2048, but that should be negligible.
Fixes: d4e0fe01a38a ("igbvf: add new driver to support 82576 virtual functions")
Signed-off-by: Akihiko Odaki <[email protected]>
---
drivers/net/ethernet/intel/igbvf/netdev.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
index 3a32809510fc..b6bca78198fa 100644
--- a/drivers/net/ethernet/intel/igbvf/netdev.c
+++ b/drivers/net/ethernet/intel/igbvf/netdev.c
@@ -1341,10 +1341,9 @@ static void igbvf_setup_srrctl(struct igbvf_adapter *adapter)
srrctl |= E1000_SRRCTL_DROP_EN;
/* Setup buffer sizes */
- srrctl |= ALIGN(adapter->rx_buffer_len, 1024) >>
- E1000_SRRCTL_BSIZEPKT_SHIFT;
+ srrctl |= adapter->rx_buffer_len >> E1000_SRRCTL_BSIZEPKT_SHIFT;
- if (adapter->rx_buffer_len < 2048) {
+ if (adapter->rx_buffer_len <= 2048) {
adapter->rx_ps_hdr_size = 0;
srrctl |= E1000_SRRCTL_DESCTYPE_ADV_ONEBUF;
} else {
@@ -1625,7 +1624,7 @@ static int igbvf_sw_init(struct igbvf_adapter *adapter)
struct net_device *netdev = adapter->netdev;
s32 rc;
- adapter->rx_buffer_len = ETH_FRAME_LEN + VLAN_HLEN + ETH_FCS_LEN;
+ adapter->rx_buffer_len = ALIGN(ETH_FRAME_LEN + VLAN_HLEN + ETH_FCS_LEN, 1024);
adapter->rx_ps_hdr_size = 0;
adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
@@ -2429,6 +2428,8 @@ static int igbvf_change_mtu(struct net_device *netdev, int new_mtu)
adapter->rx_buffer_len = ETH_FRAME_LEN + VLAN_HLEN +
ETH_FCS_LEN;
+ adapter->rx_buffer_len = ALIGN(adapter->rx_buffer_len, 1024);
+
netdev_dbg(netdev, "changing MTU from %d to %d\n",
netdev->mtu, new_mtu);
netdev->mtu = new_mtu;
--
2.39.0
Dear Akihiko,
Thank you for your patch.
Am 24.01.23 um 05:39 schrieb Akihiko Odaki:
Maybe improve the commit message summary to be more specific:
igbvf: Align rx_buffer_len to fix memory corrption
> When rx_buffer_len is not aligned by 1024, igbvf sets the aligned size
> to SRRCTL while the buffer is allocated with the unaligned size. This
> allows the device to write more data than rx_buffer_len, resulting in
> memory corruption. Align rx_buffer_len itself so that the buffer will
> be allocated with the aligned size.
>
> The condition to split RX packet header, which uses rx_buffer_len, is
> also modified so that it doesn't change the behavior for the same
> actual (unaligned) packet size. Actually the new condition is not
> identical with the old one as it will no longer request splitting when
> the actual packet size is exactly 2048, but that should be negligible.
Is there an easy way to reproduce it?
Kind regards,
Paul
> Fixes: d4e0fe01a38a ("igbvf: add new driver to support 82576 virtual functions")
> Signed-off-by: Akihiko Odaki <[email protected]>
> ---
> drivers/net/ethernet/intel/igbvf/netdev.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
> index 3a32809510fc..b6bca78198fa 100644
> --- a/drivers/net/ethernet/intel/igbvf/netdev.c
> +++ b/drivers/net/ethernet/intel/igbvf/netdev.c
> @@ -1341,10 +1341,9 @@ static void igbvf_setup_srrctl(struct igbvf_adapter *adapter)
> srrctl |= E1000_SRRCTL_DROP_EN;
>
> /* Setup buffer sizes */
> - srrctl |= ALIGN(adapter->rx_buffer_len, 1024) >>
> - E1000_SRRCTL_BSIZEPKT_SHIFT;
> + srrctl |= adapter->rx_buffer_len >> E1000_SRRCTL_BSIZEPKT_SHIFT;
>
> - if (adapter->rx_buffer_len < 2048) {
> + if (adapter->rx_buffer_len <= 2048) {
> adapter->rx_ps_hdr_size = 0;
> srrctl |= E1000_SRRCTL_DESCTYPE_ADV_ONEBUF;
> } else {
> @@ -1625,7 +1624,7 @@ static int igbvf_sw_init(struct igbvf_adapter *adapter)
> struct net_device *netdev = adapter->netdev;
> s32 rc;
>
> - adapter->rx_buffer_len = ETH_FRAME_LEN + VLAN_HLEN + ETH_FCS_LEN;
> + adapter->rx_buffer_len = ALIGN(ETH_FRAME_LEN + VLAN_HLEN + ETH_FCS_LEN, 1024);
> adapter->rx_ps_hdr_size = 0;
> adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
> adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
> @@ -2429,6 +2428,8 @@ static int igbvf_change_mtu(struct net_device *netdev, int new_mtu)
> adapter->rx_buffer_len = ETH_FRAME_LEN + VLAN_HLEN +
> ETH_FCS_LEN;
>
> + adapter->rx_buffer_len = ALIGN(adapter->rx_buffer_len, 1024);
> +
> netdev_dbg(netdev, "changing MTU from %d to %d\n",
> netdev->mtu, new_mtu);
> netdev->mtu = new_mtu;
On 2023/01/24 19:49, Paul Menzel wrote:
> Dear Akihiko,
>
>
> Thank you for your patch.
>
> Am 24.01.23 um 05:39 schrieb Akihiko Odaki:
>
> Maybe improve the commit message summary to be more specific:
>
> igbvf: Align rx_buffer_len to fix memory corrption
>
>> When rx_buffer_len is not aligned by 1024, igbvf sets the aligned size
>> to SRRCTL while the buffer is allocated with the unaligned size. This
>> allows the device to write more data than rx_buffer_len, resulting in
>> memory corruption. Align rx_buffer_len itself so that the buffer will
>> be allocated with the aligned size.
>>
>> The condition to split RX packet header, which uses rx_buffer_len, is
>> also modified so that it doesn't change the behavior for the same
>> actual (unaligned) packet size. Actually the new condition is not
>> identical with the old one as it will no longer request splitting when
>> the actual packet size is exactly 2048, but that should be negligible.
>
> Is there an easy way to reproduce it?
>
>
> Kind regards,
>
> Paul
>
>
I withdraw this patch. While igbvf sets a value greater than the actual
buffer size to SRRCTL.BSIZEPKT, such a long packet should be dropped
according to VMOLR.RLPML.
Regards,
Akihiko Odaki
>> Fixes: d4e0fe01a38a ("igbvf: add new driver to support 82576 virtual
>> functions")
>> Signed-off-by: Akihiko Odaki <[email protected]>
>> ---
>> drivers/net/ethernet/intel/igbvf/netdev.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c
>> b/drivers/net/ethernet/intel/igbvf/netdev.c
>> index 3a32809510fc..b6bca78198fa 100644
>> --- a/drivers/net/ethernet/intel/igbvf/netdev.c
>> +++ b/drivers/net/ethernet/intel/igbvf/netdev.c
>> @@ -1341,10 +1341,9 @@ static void igbvf_setup_srrctl(struct
>> igbvf_adapter *adapter)
>> srrctl |= E1000_SRRCTL_DROP_EN;
>> /* Setup buffer sizes */
>> - srrctl |= ALIGN(adapter->rx_buffer_len, 1024) >>
>> - E1000_SRRCTL_BSIZEPKT_SHIFT;
>> + srrctl |= adapter->rx_buffer_len >> E1000_SRRCTL_BSIZEPKT_SHIFT;
>> - if (adapter->rx_buffer_len < 2048) {
>> + if (adapter->rx_buffer_len <= 2048) {
>> adapter->rx_ps_hdr_size = 0;
>> srrctl |= E1000_SRRCTL_DESCTYPE_ADV_ONEBUF;
>> } else {
>> @@ -1625,7 +1624,7 @@ static int igbvf_sw_init(struct igbvf_adapter
>> *adapter)
>> struct net_device *netdev = adapter->netdev;
>> s32 rc;
>> - adapter->rx_buffer_len = ETH_FRAME_LEN + VLAN_HLEN + ETH_FCS_LEN;
>> + adapter->rx_buffer_len = ALIGN(ETH_FRAME_LEN + VLAN_HLEN +
>> ETH_FCS_LEN, 1024);
>> adapter->rx_ps_hdr_size = 0;
>> adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
>> adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
>> @@ -2429,6 +2428,8 @@ static int igbvf_change_mtu(struct net_device
>> *netdev, int new_mtu)
>> adapter->rx_buffer_len = ETH_FRAME_LEN + VLAN_HLEN +
>> ETH_FCS_LEN;
>> + adapter->rx_buffer_len = ALIGN(adapter->rx_buffer_len, 1024);
>> +
>> netdev_dbg(netdev, "changing MTU from %d to %d\n",
>> netdev->mtu, new_mtu);
>> netdev->mtu = new_mtu;