netdev - Re: [Intel-wired-lan] [PATCH RESEND] igbvf: Fix rx_buffer

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

Date:   Wed, 25 Jan 2023 13:04:10 +0900
From:   Akihiko Odaki <akihiko.odaki@...nix.com>
To:     Paul Menzel <pmenzel@...gen.mpg.de>
Cc:     Tony Nguyen <anthony.l.nguyen@...el.com>, netdev@...r.kernel.org,
        Yan Vugenfirer <yvugenfi@...hat.com>,
        linux-kernel@...r.kernel.org,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Yuri Benditovich <yuri.benditovich@...nix.com>,
        Eric Dumazet <edumazet@...gle.com>,
        intel-wired-lan@...ts.osuosl.org, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [Intel-wired-lan] [PATCH RESEND] igbvf: Fix rx_buffer_len

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 <akihiko.odaki@...nix.com>
>> ---
>>   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;