On Thu, Jan 7, 2021 at 8:33 AM Daniel Borkmann <dan...@iogearbox.net> wrote:
>
> On 1/7/21 2:05 PM, Willem de Bruijn wrote:
> > On Thu, Jan 7, 2021 at 7:52 AM Daniel Borkmann <dan...@iogearbox.net> wrote:
> >> On 1/7/21 12:40 PM, Dongseok Yi wrote:
> >>> On 2021-01-07 20:05, Daniel Borkmann wrote:
> >>>> On 1/7/21 1:39 AM, Dongseok Yi wrote:
> >>>>> skbs in fraglist could be shared by a BPF filter loaded at TC. It
> >>>>> triggers skb_ensure_writable -> pskb_expand_head ->
> >>>>> skb_clone_fraglist -> skb_get on each skb in the fraglist.
> >>>>>
> >>>>> While tcpdump, sk_receive_queue of PF_PACKET has the original fraglist.
> >>>>> But the same fraglist is queued to PF_INET (or PF_INET6) as the fraglist
> >>>>> chain made by skb_segment_list.
> >>>>>
> >>>>> If the new skb (not fraglist) is queued to one of the sk_receive_queue,
> >>>>> multiple ptypes can see this. The skb could be released by ptypes and
> >>>>> it causes use-after-free.
> >>>>>
> >>>>> [ 4443.426215] ------------[ cut here ]------------
> >>>>> [ 4443.426222] refcount_t: underflow; use-after-free.
> >>>>> [ 4443.426291] WARNING: CPU: 7 PID: 28161 at lib/refcount.c:190
> >>>>> refcount_dec_and_test_checked+0xa4/0xc8
> >>>>> [ 4443.426726] pstate: 60400005 (nZCv daif +PAN -UAO)
> >>>>> [ 4443.426732] pc : refcount_dec_and_test_checked+0xa4/0xc8
> >>>>> [ 4443.426737] lr : refcount_dec_and_test_checked+0xa0/0xc8
> >>>>> [ 4443.426808] Call trace:
> >>>>> [ 4443.426813] refcount_dec_and_test_checked+0xa4/0xc8
> >>>>> [ 4443.426823] skb_release_data+0x144/0x264
> >>>>> [ 4443.426828] kfree_skb+0x58/0xc4
> >>>>> [ 4443.426832] skb_queue_purge+0x64/0x9c
> >>>>> [ 4443.426844] packet_set_ring+0x5f0/0x820
> >>>>> [ 4443.426849] packet_setsockopt+0x5a4/0xcd0
> >>>>> [ 4443.426853] __sys_setsockopt+0x188/0x278
> >>>>> [ 4443.426858] __arm64_sys_setsockopt+0x28/0x38
> >>>>> [ 4443.426869] el0_svc_common+0xf0/0x1d0
> >>>>> [ 4443.426873] el0_svc_handler+0x74/0x98
> >>>>> [ 4443.426880] el0_svc+0x8/0xc
> >>>>>
> >>>>> Fixes: 3a1296a38d0c (net: Support GRO/GSO fraglist chaining.)
> >>>>> Signed-off-by: Dongseok Yi <dseok...@samsung.com>
> >>>>> Acked-by: Willem de Bruijn <will...@google.com>
> >>>>> ---
> >>>>> net/core/skbuff.c | 20 +++++++++++++++++++-
> >>>>> 1 file changed, 19 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> v2: Expand the commit message to clarify a BPF filter loaded
> >>>>>
> >>>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> >>>>> index f62cae3..1dcbda8 100644
> >>>>> --- a/net/core/skbuff.c
> >>>>> +++ b/net/core/skbuff.c
> >>>>> @@ -3655,7 +3655,8 @@ struct sk_buff *skb_segment_list(struct sk_buff
> >>>>> *skb,
> >>>>> unsigned int delta_truesize = 0;
> >>>>> unsigned int delta_len = 0;
> >>>>> struct sk_buff *tail = NULL;
> >>>>> - struct sk_buff *nskb;
> >>>>> + struct sk_buff *nskb, *tmp;
> >>>>> + int err;
> >>>>>
> >>>>> skb_push(skb, -skb_network_offset(skb) + offset);
> >>>>>
> >>>>> @@ -3665,11 +3666,28 @@ struct sk_buff *skb_segment_list(struct sk_buff
> >>>>> *skb,
> >>>>> nskb = list_skb;
> >>>>> list_skb = list_skb->next;
> >>>>>
> >>>>> + err = 0;
> >>>>> + if (skb_shared(nskb)) {
> >>>>> + tmp = skb_clone(nskb, GFP_ATOMIC);
> >>>>> + if (tmp) {
> >>>>> + kfree_skb(nskb);
> >>>>
> >>>> Should use consume_skb() to not trigger skb:kfree_skb tracepoint when
> >>>> looking
> >>>> for drops in the stack.
> >>>
> >>> I will use to consume_skb() on the next version.
> >>>
> >>>>> + nskb = tmp;
> >>>>> + err = skb_unclone(nskb, GFP_ATOMIC);
> >>>>
> >>>> Could you elaborate why you also need to unclone? This looks odd here.
> >>>> tc layer
> >>>> (independent of BPF) from ingress & egress side generally assumes
> >>>> unshared skb,
> >>>> so above clone + dropping ref of nskb looks okay to make the main skb
> >>>> struct private
> >>>> for mangling attributes (e.g. mark) & should suffice. What is the exact
> >>>> purpose of
> >>>> the additional skb_unclone() in this context?
> >>>
> >>> Willem de Bruijn said:
> >>> udp_rcv_segment later converts the udp-gro-list skb to a list of
> >>> regular packets to pass these one-by-one to udp_queue_rcv_one_skb.
> >>> Now all the frags are fully fledged packets, with headers pushed
> >>> before the payload.
> >>
> >> Yes.
> >>
> >>> PF_PACKET handles untouched fraglist. To modify the payload only
> >>> for udp_rcv_segment, skb_unclone is necessary.
> >>
> >> I don't parse this last sentence here, please elaborate in more detail on
> >> why
> >> it is necessary.
> >>
> >> For example, if tc layer would modify mark on the skb, then
> >> __copy_skb_header()
> >> in skb_segment_list() will propagate it. If tc layer would modify payload,
> >> the
> >> skb_ensure_writable() will take care of that internally and if needed pull
> >> in
> >> parts from fraglist into linear section to make it private. The purpose of
> >> the
> >> skb_clone() above iff shared is to make the struct itself private (to
> >> safely
> >> modify its struct members). What am I missing?
> >
> > If tc writes, it will call skb_make_writable and thus pskb_expand_head
> > to create a private linear section for the head_skb.
> >
> > skb_segment_list overwrites part of the skb linear section of each
> > fragment itself. Even after skb_clone, the frag_skbs share their
> > linear section with their clone in pf_packet, so we need a
> > pskb_expand_head here, too.
>
> Ok, got it, thanks for the explanation. Would be great to have it in the v3
> commit
> log as well as that was more clear than the above. Too bad in that case (otoh
> the pf_packet situation could be considered corner case ...); ether way, fix
> makes
> sense then.
Thanks for double checking the tricky logic. Pf_packet + BPF is indeed
a peculiar corner case. But there are perhaps more, like raw sockets,
and other BPF hooks that can trigger an skb_make_writable.
Come to think of it, the no touching shared data rule is also violated
without a BPF program? Then there is nothing in the frag_skbs
themselves signifying that they are shared, but the head_skb is
cloned, so its data may still not be modified.
This modification happens to be safe in practice, as the pf_packet
clones don't access the bytes before skb->data where this path inserts
headers. But still.
