netdev - Re: SELinux/IP_PASSSEC regression in 4.13-rcX

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: <CAHC9VhSAifQtx3eaQD42XMx8beC08JBGQ9SaKFvkD9qVsT8VCg@mail.gmail.com>
Date:   Mon, 24 Jul 2017 15:00:01 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     netdev@...r.kernel.org, selinux@...ho.nsa.gov
Subject: Re: SELinux/IP_PASSSEC regression in 4.13-rcX

On Mon, Jul 24, 2017 at 12:09 PM, Paolo Abeni <pabeni@...hat.com> wrote:
> Hi,
>
> On Mon, 2017-07-24 at 10:42 -0400, Paul Moore wrote:
>> The change in behavior for userspace makes me a little nervous as
>> there is no way of knowing how any random application may be coded.
>> Even if we are confident that the majority of applications set
>> IP_PASSSEC before calling bind(), we are likely still stuck with a few
>> that will break, and that means a lot of hard to debug problem
>> reports.
>>
>> I would feel much more comfortable if we could preserve the existing behavior.
>
> I agree, we must preserve the original behavior.
>
> Re-thinking about the problem, checking skb->sp in the BH, and storing
> the status in the scratch area should both fix the issue in a sane way
> and preserve the optimization.
>
> Something like the code below. Could you please try in your
> environment? (or point me to simple reproducer ;-)

I'm happy to test this, but if you are curious, you can find the
selinux-testsuite at the link below; the "inet_socket" tests are the
ones relevant to this problem.

* https://github.com/SELinuxProject/selinux-testsuite

However, I believe there is a problem with this patch, see below.

> ---
> diff --git a/include/net/udp.h b/include/net/udp.h
> index 972ce4b..8d2c406 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -305,33 +305,44 @@ struct sock *udp6_lib_lookup_skb(struct sk_buff *skb,
>  /* UDP uses skb->dev_scratch to cache as much information as possible and avoid
>   * possibly multiple cache miss on dequeue()
>   */
> -#if BITS_PER_LONG == 64
> -
> -/* truesize, len and the bit needed to compute skb_csum_unnecessary will be on
> - * cold cache lines at recvmsg time.
> - * skb->len can be stored on 16 bits since the udp header has been already
> - * validated and pulled.
> - */
>  struct udp_dev_scratch {
> -       u32 truesize;
> +       /* skb->truesize and the stateless bit embeded in a single field;
> +        * do not use a bitfield since the compiler emits better/smaller code
> +        * this way
> +        */
> +       u32 _tsize_state;
> +
> +#if BITS_PER_LONG == 64
> +       /* len and the bit needed to compute skb_csum_unnecessary
> +        * will be on cold cache lines at recvmsg time.
> +        * skb->len can be stored on 16 bits since the udp header has been
> +        * already validated and pulled.
> +        */
>         u16 len;
>         bool is_linear;
>         bool csum_unnecessary;
> +#endif
>  };

...

> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index b057653..d243772 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1163,34 +1163,32 @@ int udp_sendpage(struct sock *sk, struct page *page, int offset,
>         return ret;
>  }
>
> -#if BITS_PER_LONG == 64
> +#define UDP_SKB_IS_STATELESS 0x80000000
> +
>  static void udp_set_dev_scratch(struct sk_buff *skb)
>  {
> -       struct udp_dev_scratch *scratch;
> +       struct udp_dev_scratch *scratch = udp_skb_scratch(skb);
>
>         BUILD_BUG_ON(sizeof(struct udp_dev_scratch) > sizeof(long));

The BUILD_BUG_ON() assertion no longer appears to be correct with this patch.

> -       scratch = (struct udp_dev_scratch *)&skb->dev_scratch;
> -       scratch->truesize = skb->truesize;
> +       scratch->_tsize_state = skb->truesize;
> +#if BITS_PER_LONG == 64
>         scratch->len = skb->len;
>         scratch->csum_unnecessary = !!skb_csum_unnecessary(skb);
>         scratch->is_linear = !skb_is_nonlinear(skb);
> +#endif
> +       if (likely(!skb->_skb_refdst))
> +               scratch->_tsize_state |= UDP_SKB_IS_STATELESS;
>  }

-- 
paul moore
www.paul-moore.com