netdev - Re: [PATCH v13 bpf-next 09/10] bpf: Add bpf_dynptr_slice and bpf_dynptr_slice

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: <CAJnrk1Y=u_9sVo1QhNopRu7F7tRsmZmcNDMeiUw+QF3rtQQ2og@mail.gmail.com>
Date:   Mon, 27 Mar 2023 00:47:35 -0700
From:   Joanne Koong <joannelkoong@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Kumar Kartikeya Dwivedi <memxor@...il.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <martin.lau@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Network Development <netdev@...r.kernel.org>,
        Toke Høiland-Jørgensen <toke@...nel.org>
Subject: Re: [PATCH v13 bpf-next 09/10] bpf: Add bpf_dynptr_slice and bpf_dynptr_slice_rdwr

On Thu, Mar 16, 2023 at 11:55 AM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Mon, Mar 13, 2023 at 7:41 AM Kumar Kartikeya Dwivedi
> <memxor@...il.com> wrote:
> >
[...]
> > > > For bpf_dynptr_slice_rdrw we can mark buffer[] in stack as
> > > > poisoned with dynptr_id == R0's PTR_TO_MEM dynptr_id.
> > > > Then as soon as first spillable reg touches that poisoned stack area
> > > > we can invalidate all PTR_TO_MEM's with that dynptr_id.
> > >
> > > Okay, this makes sense to me. are you already currently working or
> > > planning to work on a fix for this Kumar, or should i take a stab at
> > > it?
> >
> > I'm not planning to do so, so go ahead. One more thing I noticed just now is
> > that we probably need to update regsafe to perform a check_ids comparison for
> > dynptr_id for dynptr PTR_TO_MEMs? It was not a problem back when f8064ab90d66
> > ("bpf: Invalidate slices on destruction of dynptrs on stack") was added but
> > 567da5d253cd ("bpf: improve regsafe() checks for PTR_TO_{MEM,BUF,TP_BUFFER}")
> > added PTR_TO_MEM in the switch statement.
>
> I can take care of this. But I really would like to avoid these
> special cases of extra dynptr_id, exactly for reasons like this
> omitted check.
>
> What do people think about generalizing current ref_obj_id to be more
> like "lifetime id" (to borrow Rust terminology a bit), which would be
> an object (which might or might not be a tracked reference) defining
> the scope/lifetime of the current register (whatever it represents).
>
> I haven't looked through code much, but I've been treating ref_obj_id
> as that already in my thinking before, and it seems to be a better
> approach than having a special-case of dynptr_id.
>
> Thoughts?

Thanks for taking care of this (and apologies for the late reply). i
think the dynptr_id field would still be needed in this case to
associate a slice with a dynptr, so that when a dynptr is invalidated
its slices get invalidated as well. I'm not sure we could get away
with just having ref_obj_id symbolize that in the case where the
underlying object is a tracked reference, because for example, it
seems like a dynptr would need both a unique reference id to the
object (so that if for example there are two dynptrs pointing to the
same object, they will both be assignedthe same reference id so the
object can't for example be freed twice) and also its own dynptr id so
that its slices get invalidated if the dynptr is invalidated