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

Message-ID: <93AF473E2DA327428DE3D46B72B1E9FD411D09C8@CHN-SV-EXMX02.mchp-main.com>
Date:   Tue, 30 Oct 2018 19:36:11 +0000
From:   <Tristram.Ha@...rochip.com>
To:     <Claudiu.Beznea@...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

> 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.
 
> 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?

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.