netdev - Re: [PATCH net] tcp: fix page frag corruption on page fault

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:   Fri, 26 Nov 2021 07:27:03 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     netdev@...r.kernel.org, Steffen Froemer <sfroemer@...hat.com>
Subject: Re: [PATCH net] tcp: fix page frag corruption on page fault

On Fri, Nov 26, 2021 at 7:05 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> Hello,
>
...

> Double checking I understood correctly: the problem is that in case of
> skb_copy_to_page_nocache() error, if the skb is not empty, random data
> will be introduced into the TCP stream, or something else/more? I
> obviously did not see that before the submission, nor tests cached it,
> sorry.

If there is a copy error, the skb is left with an extended fragment
(if a merge happened)
or a new frag, with random data in it, as the copy operation failed.

We are thus going to send garbage on the network on next sendmsg(),
which will append
more stuff in the frag array.

>
> > Please investigate CIFS and gfpflags_normal_context() tandem to fix
> > this issue instead.
>
> Do you mean changing gfpflags_normal_context() definition so that cifs
> allocation are excluded? Something alike the following should do:

We need to find one flag that can ask  gfpflags_normal_context() to
return false for this case.

Or if too difficult, stop only using sk->sk_allocation to decide
between the per-thread frag, or the per socket one.

I presume there are few active CIFS sockets on a host. They should use
a per socket sk_frag.



>
> ---
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index b976c4177299..f9286aeeded5 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -379,8 +379,8 @@ static inline bool gfpflags_allow_blocking(const gfp_t gfp_flags)
>   */
>  static inline bool gfpflags_normal_context(const gfp_t gfp_flags)
>  {
> -       return (gfp_flags & (__GFP_DIRECT_RECLAIM | __GFP_MEMALLOC)) ==
> -               __GFP_DIRECT_RECLAIM;
> +       return (gfp_flags & (__GFP_DIRECT_RECLAIM | __GFP_MEMALLOC | __GFP_FS)) ==
> +               (__GFP_DIRECT_RECLAIM | __GFP_FS);
>  }
> ---
> If so there is a caveat: dlm is currently using
> gfpflags_normal_context() - apparently to check for non blocking
> context. If I'll change gfpflags_normal_context() definition I likely
> will have to replace gfpflags_normal_context() with
> gfpflags_allow_blocking() in dlm.
>
> In that case the relevant patch should touch both the mm and the fs
> subsystem. In that case I guess I should go via the fs tree first and
> the via mm?
>
> Thanks!
>
> Paolo
>