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: <93AF473E2DA327428DE3D46B72B1E9FD411D076B@CHN-SV-EXMX02.mchp-main.com>
Date:   Mon, 29 Oct 2018 21:40:26 +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, please, tell me if the above variable was false in your case?
> 
> bool cloned = skb_cloned(*skb) || skb_header_cloned(*skb);
> 
> If yes, then, the proper fix would be as follows:
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c
> b/drivers/net/ethernet/cadence/macb_main.c
> index 8f5bf9166c11..492a8e1a34cd 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1690,7 +1690,7 @@ static int macb_pad_and_fcs(struct sk_buff **skb,
> struct net_device *ndev)
>                 padlen += ETH_FCS_LEN;
>         }
> 
> -       if (!cloned && headroom + tailroom >= padlen) {
> +       if (!cloned && headroom + tailroom >= ETH_FCS_LEN) {
>                 (*skb)->data = memmove((*skb)->head, (*skb)->data, (*skb)->len);
>                 skb_set_tail_pointer(*skb, (*skb)->len);
>         } else {
> 
> Could you please check if it works in your case (and without your patch)?
> 

Actually doing that reveals another bug:

	if (padlen) {
		if (padlen >= ETH_FCS_LEN)
			skb_put_zero(*skb, padlen - ETH_FCS_LEN);
		else
			skb_trim(*skb, ETH_FCS_LEN - padlen);
	}

My fix calls skb_put_zero with zero length.  Your change calls skb_trim which
actually sets the socket buffer length to 1!

When this problem happens headroom is 2, tailroom is 1, skb->len is 61, and
padlen is 3.

DSA driver is being used.  That is why the length is already padded to 60 bytes
and 1-byte tail tag is added.

BTW, I am not sure while this macb_pad_and_fcs function was added.  Is it to
workaround some hardware bugs?  The code is executed only when
NETIF_IF_HW_CSUM is used.  But if hardware tx checksumming is enabled why
not also use the hardware to calculate CRC?

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.