netdev - Re: [PATCH net] net: ethernet: cadence: fix socket buffer corruption problem

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, 31 Oct 2018 07:45:41 +0000
From:   <Claudiu.Beznea@...rochip.com>
To:     <Tristram.Ha@...rochip.com>
CC:     <UNGLinuxDriver@...rochip.com>, <netdev@...r.kernel.org>,
        <davem@...emloft.net>, <Nicolas.Ferre@...rochip.com>
Subject: Re: [PATCH net] net: ethernet: cadence: fix socket buffer corruption
 problem



On 30.10.2018 21:36, Tristram Ha - C24268 wrote:
>> Could you check on your side that applying this on top of your patch, your
>> scenario is still working?
>>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 1d86b4d5645a..e1347d6d1b50 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -1702,12 +1702,8 @@ static int macb_pad_and_fcs(struct sk_buff **skb,
>> struct net_device *ndev)
>>                 *skb = nskb;
>>         }
>>
>> -       if (padlen) {
>> -               if (padlen >= ETH_FCS_LEN)
>> -                       skb_put_zero(*skb, padlen - ETH_FCS_LEN);
>> -               else
>> -                       skb_trim(*skb, ETH_FCS_LEN - padlen);
>> -       }
>> +       if (padlen > ETH_FCS_LEN)
>> +               skb_put_zero(*skb, padlen - ETH_FCS_LEN);
> 
> I think it is okay but I need to check all paths are covered.

On my side I checked with pktgen generating packets with sizes starting
from 1-MTU. Same scripts I also used when I first implemented this but it
seems that your scenario wasn't covered. Please have a look and let me know
how it works on your side.

> 
>> It was reported in [1] that UDP checksum is offloaded to hardware no matter
>> the application previously computed it.
>>
>> The code should be executed only for packets that has checksum computed
>> by
>> applications ((*skb)->ip_summed != CHECKSUM_PARTIAL). The idea was to
>> not
>> recompute checksum for packets with checksum already computed. To do
>> so,
>> while hardware checksum is enabled (NETIF_F_HW_CSUM), TX_NOCRC bit
>> should
>> be set on buffer descriptor. But to do so, packets must have a minimum size
>> of 64 and FCS to be computed.
>>
>> The NETIF_F_HW_CSUM check was placed there because the issue
>> described in
>> [1] is reproducible because hardware checksum is enabled and overrides the
>> checksum provided by applications.
>>
>> [1] https://www.spinics.net/lists/netdev/msg505065.html
> 
> I understand the issue now.  It is weird that the transmit descriptor does not
> have direct control over turning on checksum generation or not, but it wastes
> 3 bits returning the error codes of such generation.  What can the driver do
> with such information?

Yep, from my POV it would have been nice if it could have been able to do
the pad an FCS even when hardware checksum is enabled and TX_NOCRC bit is
set in descriptor. For cases when hardware checksum is disable it seems
that it could handle it.

> 
> In my opinion then hardware transmit checksumming cannot be supported
> In Linux.
> 
>>> NETIF_F_SG is not enabled in the MAC I used, so enabling
>> NETIF_IF_HW_CSUM
>>> is rather pointless.  With the padding code the transmit throughput cannot
>> get
>>> higher than 100Mbps in a gigabit connection.
>>>
>>> I would recommend to add this option to disable manual padding in one of
>> those
>>> macb_config structures.
>>
>> In this way the user would have to know from the beginning what kind of
>> packets are used.
>>
> 
> The kernel already does a good job of calculating checksum.  Using hardware to
> do that does not improve performance much.
> 
> Alternative is to use ethtool to disable hardware tx checksum so that software can
> intentionally send wrong checksums.
>