Re: [PATCH v3] bpf: Fix KASAN use-after-free Read in compute_effective_progs

From: Tadeusz Struk <tadeusz.struk@linaro.org>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@kernel.org>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	linux- stable <stable@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>,
	syzbot+f264bffdfbd5614f3bb2@syzkaller.appspotmail.com
Subject: Re: [PATCH v3] bpf: Fix KASAN use-after-free Read in compute_effective_progs
Date: Mon, 16 May 2022 16:35:06 -0700	[thread overview]
Message-ID: <2fcdbecf-5352-ea81-ee42-ee10fbe2f72e@linaro.org> (raw)
In-Reply-To: <CAEf4BzY-p13huoqo6N7LJRVVj8rcjPeP3Cp=KDX4N2x9BkC9Zw@mail.gmail.com>

On 5/16/22 16:16, Andrii Nakryiko wrote:
> On Fri, May 13, 2022 at 12:08 PM Tadeusz Struk <tadeusz.struk@linaro.org> wrote:
>>   kernel/bpf/cgroup.c | 64 +++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 56 insertions(+), 8 deletions(-)
>>
>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>> index 128028efda64..9d3af4d6c055 100644
>> --- a/kernel/bpf/cgroup.c
>> +++ b/kernel/bpf/cgroup.c
>> @@ -681,6 +681,57 @@ static struct bpf_prog_list *find_detach_entry(struct list_head *progs,
>>          return ERR_PTR(-ENOENT);
>>   }
>>
>> +/**
>> + * purge_effective_progs() - After compute_effective_progs fails to alloc new
>> + *                           cgrp->bpf.inactive table we can recover by
>> + *                           recomputing the array in place.
>> + *
>> + * @cgrp: The cgroup which descendants to traverse
>> + * @link: A link to detach
>> + * @atype: Type of detach operation
>> + */
>> +static void purge_effective_progs(struct cgroup *cgrp, struct bpf_prog *prog,
>> +                                 enum cgroup_bpf_attach_type atype)
>> +{
>> +       struct cgroup_subsys_state *css;
>> +       struct bpf_prog_array_item *item;
>> +       struct bpf_prog *tmp;
>> +       struct bpf_prog_array *array;
>> +       int index = 0, index_purge = -1;
>> +
>> +       if (!prog)
>> +               return;
>> +
>> +       /* recompute effective prog array in place */
>> +       css_for_each_descendant_pre(css, &cgrp->self) {
>> +               struct cgroup *desc = container_of(css, struct cgroup, self);
>> +
>> +               array = desc->bpf.effective[atype];
> 
> ../kernel/bpf/cgroup.c:748:23: warning: incorrect type in assignment
> (different address spaces)
> ../kernel/bpf/cgroup.c:748:23:    expected struct bpf_prog_array *array
> ../kernel/bpf/cgroup.c:748:23:    got struct bpf_prog_array [noderef] __rcu *
> 
> 
> you need rcu_dereference here? but also see suggestions below to avoid
> iterating effective directly (it's ambiguous to search by prog only)

I didn't check it with sparse so I didn't see this warning.
Will fix in the next version.

> 
>> +               item = &array->items[0];
>> +
>> +               /* Find the index of the prog to purge */
>> +               while ((tmp = READ_ONCE(item->prog))) {
>> +                       if (tmp == prog) {
> 
> I think comparing just prog isn't always correct, as the same program
> can be in effective array multiple times if attached through bpf_link.
> 
> Looking at replace_effective_prog() I think we can do a very similar
> (and tested) approach:
> 
> 1. restore original pl state in __cgroup_bpf_detach (so we can find it
> by comparing pl->prog == prog && pl->link == link)
> 2. use replace_effective_prog's approach to find position of pl in
> effective array (using this nested for loop over cgroup parents and
> list_for_each_entry inside)
> 3. then instead of replacing one prog with another do
> bpf_prog_array_delete_safe_at ?
> 
> I'd feel more comfortable using the same tested overall approach of
> replace_effective_prog.

Ok, I can try that.

> 
>> +                               index_purge = index;
>> +                               break;
>> +                       }
>> +                       item++;
>> +                       index++;
>> +               }
>> +
>> +               /* Check if we found what's needed for removing the prog */
>> +               if (index_purge == -1 || index_purge == index - 1)
>> +                       continue;
> 
> the search shouldn't fail, should it?

I wasn't if the prog will be present in all parents so I decided to add this
check to make sure it is found.

> 
>> +
>> +               /* Remove the program from the array */
>> +               WARN_ONCE(bpf_prog_array_delete_safe_at(array, index_purge),
>> +                         "Failed to purge a prog from array at index %d", index_purge);
>> +
>> +               index = 0;
>> +               index_purge = -1;
>> +       }
>> +}
>> +
>>   /**
>>    * __cgroup_bpf_detach() - Detach the program or link from a cgroup, and
>>    *                         propagate the change to descendants
>> @@ -723,8 +774,11 @@ static int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog,
>>          pl->link = NULL;
>>
>>          err = update_effective_progs(cgrp, atype);
>> -       if (err)
>> -               goto cleanup;
>> +       if (err) {
>> +               struct bpf_prog *prog_purge = prog ? prog : link->link.prog;
>> +
> 
> so here we shouldn't forget link, instead pass both link and prog (one
> of them will have to be NULL) into purge_effective_progs

ok, I will pass in both.

-- 
Thanks,
Tadeusz