netdev - Re: [PATCH net] Revert "net: macsec: use skb_ensure_writable_head

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: <878r4pov1b.fsf@nvidia.com>
Date: Tue, 16 Jan 2024 12:45:46 -0800
From: Rahul Rameshbabu <rrameshbabu@...dia.com>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, Gal Pressman
 <gal@...dia.com>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
 <kuba@...nel.org>, Radu Pirea <radu-nicolae.pirea@....nxp.com>, "David S .
 Miller" <davem@...emloft.net>
Subject: Re: [PATCH net] Revert "net: macsec: use
 skb_ensure_writable_head_tail to expand the skb"

On Tue, 16 Jan, 2024 14:51:19 +0100 Sabrina Dubroca <sd@...asysnail.net> wrote:
> 2024-01-16, 11:39:35 +0100, Paolo Abeni wrote:
>> On Sun, 2024-01-14 at 09:42 -0800, Rahul Rameshbabu wrote:
>> > This reverts commit b34ab3527b9622ca4910df24ff5beed5aa66c6b5.
>> > 
>> > Using skb_ensure_writable_head_tail without a call to skb_unshare causes
>> > the MACsec stack to operate on the original skb rather than a copy in the
>> > macsec_encrypt path. This causes the buffer to be exceeded in space, and
>> > leads to warnings generated by skb_put operations. 
>> 
>> This part of the changelog is confusing to me. It looks like the skb
>> should be uncloned under the same conditions before and after this
>> patch (and/or the reverted)??!
>
> I don't think so. The old code was doing unshare +
> expand. skb_ensure_writable_head_tail calls pskb_expand_head without
> unshare, which doesn't give us a fresh sk_buff, only takes care of the
> headroom/tailroom. Or do I need more coffee? :/

Sabrina's analysis is correct. We no longer get a fresh sk_buff with
this commit.

>
>> Possibly dev->needed_headroom/needed_tailroom values are incorrect?!?
>
> That's also possible following commit a73d8779d61a ("net: macsec:
> introduce mdo_insert_tx_tag"). Then this revert would only be hiding
> the issue.

Ah, I think that is an interesting point.

    static void macsec_set_head_tail_room(struct net_device *dev)
    {
    	struct macsec_dev *macsec = macsec_priv(dev);
    	struct net_device *real_dev = macsec->real_dev;
    	int needed_headroom, needed_tailroom;
    	const struct macsec_ops *ops;

    	ops = macsec_get_ops(macsec, NULL);
    	if (ops) {

This condition should really be ops && ops->mdo_insert_tx_tags. Let me
retest with this change and post back. That said, I am wondering if we
still need a fresh skb in the macsec stack or not as was done previously
with skb_unshare/skb_copy_expand or not.

    		needed_headroom = ops->needed_headroom;
    		needed_tailroom = ops->needed_tailroom;
    	} else {
    		needed_headroom = MACSEC_NEEDED_HEADROOM;
    		needed_tailroom = MACSEC_NEEDED_TAILROOM;
    	}

    	dev->needed_headroom = real_dev->needed_headroom + needed_headroom;
    	dev->needed_tailroom = real_dev->needed_tailroom + needed_tailroom;
    }

--
Thanks,

Rahul Rameshbabu