netdev - Re: [PATCH net-next 1/8] net: Change SKB_GSO_DODGY to be a tx

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]

Date:	Thu, 16 Jun 2016 13:33:26 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Tom Herbert <tom@...bertland.com>
Cc:	David Miller <davem@...emloft.net>,
	Netdev <netdev@...r.kernel.org>, Kernel Team <kernel-team@...com>
Subject: Re: [PATCH net-next 1/8] net: Change SKB_GSO_DODGY to be a tx_flag

On Thu, Jun 16, 2016 at 1:18 PM, Tom Herbert <tom@...bertland.com> wrote:
> On Thu, Jun 16, 2016 at 11:58 AM, Alexander Duyck
> <alexander.duyck@...il.com> wrote:
>> On Thu, Jun 16, 2016 at 10:51 AM, Tom Herbert <tom@...bertland.com> wrote:
>>> This replaces gso_type SKB_GSO_DODGY with a new tx_flag named
>>> SKBTX_UNTRUSTED_SOURCE. This more generically desrcibes the skb
>>> being created from a untrusted source as a characteristic of and skbuff.
>>> This also frees up one gso_type flag bit.
>>>
>>> Signed-off-by: Tom Herbert <tom@...bertland.com>
>>
>> Instead of leaving this bit in the shared_info why not look at moving
>> it into the sk_buff itself?  It seems like this might be a better
>> candidate for something like that as a large part of what the dodgy
>> bit represents is that the header offsets are likely not set correctly
>> and need to be parsed out and updated.  It might make more sense to
>> place this in the slot just after remcsum_offload.  That way once all
>> the header offsets have been updated you could just update this one
>> bit to indicate that the header offsets stored in this sk_buff are
>> valid.
>>
>> I also don't see where these changes address any changes needed to
>> skb_gso_ok in order to actually trigger the partial walk though the
>> GSO code.  You probably need to look at adding a statement there to do
>> a check for your untrusted source bit versus the GSO_ROBUST feature.
>> It probably doesn't need to be much, just something like tacking on a
>> "&& (!skb_is_untrustued(skb) || (features & NETIF_F_GSO_ROBUST)" to
>> the conditional statement.
>>
> All the places where SKB_GSO_DODGY was being checked should have been
> replaced with SKBTX_UNTRUSTED_SOURCE.

Yes and no.  So net_gso_ok used to be one of those places but you
moved the one bit and dropped the other so is it no longer is being
checked there.

That is why I mentioned looking at adding one additional check for the
untrusted bit and NETIF_F_GSO_ROBUST.  You could probably just tack it
on at the end of the features check at the end of net_gso_ok.

- Alex