lkml.org 
[lkml]   [2022]   [May]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3] bpf: Fix KASAN use-after-free Read in compute_effective_progs
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

\
 
 \ /
  Last update: 2022-05-17 01:35    [W:0.718 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site