netdev - Re: KCM - recvmsg() mangles packets?

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPDqMeptLoLZ2RU+T-d7YwkO2AeKV+nfow4C-LqqeZd9mo-vMg@mail.gmail.com>
Date:   Thu, 9 Aug 2018 13:58:30 -0700
From:   Tom Herbert <tom@...ntonium.net>
To:     Dominique Martinet <asmadeus@...ewreck.org>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: KCM - recvmsg() mangles packets?

On Sun, Aug 5, 2018 at 4:39 PM, Dominique Martinet
<asmadeus@...ewreck.org> wrote:
> Dominique Martinet wrote on Sun, Aug 05, 2018:
>> It's getting late but I'll try adding a pskb_pull in there tomorrow, it
>> would be better to make the bpf program start with an offset but I don't
>> think that'll be easy to change...
>
> I can confirm the following patch fixes the issue for me:
> -----8<---------------------
> diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
> index 625acb27efcc..348ff5945591 100644
> --- a/net/strparser/strparser.c
> +++ b/net/strparser/strparser.c
> @@ -222,6 +222,16 @@ static int __strp_recv(read_descriptor_t *desc, struct sk_buff *orig_skb,
>                 if (!stm->strp.full_len) {
>                         ssize_t len;
>
> +                       /* Can only parse if there is no offset */
> +                       if (unlikely(stm->strp.offset)) {
> +                               if (!pskb_pull(skb, stm->strp.offset)) {
> +                                       STRP_STATS_INCR(strp->stats.mem_fail);
> +                                       strp_parser_err(strp, -ENOMEM, desc);
> +                                       break;
> +                               }
> +                               stm->strp.offset = 0;
> +                       }
> +

Seems okay to me for a fix. Looks like strp.offset is only set in one
place and read in one place. With this pull maybe that just can go
away?

Tom


>                         len = (*strp->cb.parse_msg)(strp, head);
>
>                         if (!len) {
> ----------------8<----------------------
>
> Now, I was looking at other users of strparser (I see sockmap, kcm and
> tls) and it looks like sockmap does not handle offsets either but tls
> does by using skb_copy_bits -- they're copying the tls header to a
> buffer on the stack.
>
> kcm cannot do that because we do not know how much data the user expects
> to read, and I'm not comfortable doing pskb_pull in the kcm callback
> either, but the cost of this pull is probably non-negligible if some
> user can make do without it...
>
> On the other hand, I do not see how to make the bpf program handle an
> offset in the skb as that offset is strparser-specific.
>
> Maybe add a flag in the cb that specifies wether the callback allows
> non-zero offset?
>
>
> I'll let you see if you can reproduce this and will wait for advices on
> how to solve this properly so we can work on a proper fix.
>
>
> Thanks,
> --
> Dominique