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

Message-ID: <87y1coohq9.fsf@nvidia.com>
Date: Tue, 16 Jan 2024 17:22:58 -0800
From: Rahul Rameshbabu <rrameshbabu@...dia.com>
To: Rahul Rameshbabu <rrameshbabu@...dia.com>
Cc: Sabrina Dubroca <sd@...asysnail.net>, 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 12:45:46 -0800 Rahul Rameshbabu <rrameshbabu@...dia.com> wrote:
> 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.

Both fixing the headroom/tailroom management in this commit,
a73d8779d61a ("net: macsec: introduce mdo_insert_tx_tag"), as well as
simply reverting this commit does not resolve the issue. I also end up
needing to revert b34ab3527b96 ("net: macsec: use
skb_ensure_writable_head_tail to expand the skb"), so that a fresh
sk_buff is created to avoid the panic mentioned in this commit.

I think we can do one of two things.

1. We merge this patch, and I send a follow-up fix with regards to the
   issues in b34ab3527b96.
2. I send a v2 where I add an additional patch for fixing the issues in
   b34ab3527b96.

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