| Message ID | 20230821000230.3108635-1-yonghong.song@linux.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf,1/2] bpf: Fix a bpf_kptr_xchg() issue with local kptr | expand |
On Mon, 21 Aug 2023 at 05:33, Yonghong Song <yonghong.song@linux.dev> wrote: > > When reviewing local percpu kptr support, Alexei discovered a bug > wherea bpf_kptr_xchg() may succeed even if the map value kptr type and > locally allocated obj type do not match ([1]). Missed struct btf_id > comparison is the reason for the bug. This patch added such struct btf_id > comparison and will flag verification failure if types do not match. > > [1] https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t > > Reported-by: Alexei Starovoitov <ast@kernel.org> > Fixes: 738c96d5e2e3 ("bpf: Allow local kptrs to be exchanged via bpf_kptr_xchg") > Signed-off-by: Yonghong Song <yonghong.song@linux.dev> > --- Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> But some comments below... > kernel/bpf/verifier.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 02a021c524ab..4e1ecd4b8497 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -7745,7 +7745,18 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n"); > return -EFAULT; > } > - /* Handled by helper specific checks */ > + if (meta->func_id == BPF_FUNC_kptr_xchg) { > + struct btf_field *kptr_field = meta->kptr_field; > + > + if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off, > + kptr_field->kptr.btf, kptr_field->kptr.btf_id, > + true)) { > + verbose(env, "R%d is of type %s but %s is expected\n", > + regno, btf_type_name(reg->btf, reg->btf_id), > + btf_type_name(kptr_field->kptr.btf, kptr_field->kptr.btf_id)); > + return -EACCES; > + } > + } The fix on its own looks ok to me, but any reason you'd not like to delegate to map_kptr_match_type? Just to collect kptr related type matching logic in its own place. It doesn't matter too much though. While looking at the code, I noticed one more problem. I don't think the current code is enforcing that 'reg->off is zero' assumption when releasing MEM_ALLOC types. We are only saved because you passed strict=true which makes passing non-zero reg->off a noop and returns false. The hunk was added to check_func_arg_reg_off in 6a3cd3318ff6 ("bpf: Migrate release_on_unlock logic to non-owning ref semantics") which bypasses in case type is MEM_ALLOC and offset points to some graph root or node. I am not sure why this check exists, IIUC rbtree release helpers are not tagged KF_RELEASE, and no release helper can type match on them either. Dave, can you confirm? I think it might be an accidental leftover of some refactoring. In fact, it seems bpf_obj_drop is already broken because reg->off assumption is violated. For node_data type: bpf_obj_drop(&res->data); returns R1 must have zero offset when passed to release func No graph node or root found at R1 type:node_data off:8 but bpf_obj_drop(&res->node); passes verifier. 15: (bf) r1 = r0 ; R0_w=ptr_node_data(ref_obj_id=3,off=16,imm=0) R1_w=ptr_node_data(ref_obj_id=3,off=16,imm=0) refs=3 16: (b7) r2 = 0 ; R2_w=0 refs=3 17: (85) call bpf_obj_drop_impl#74867 ; safe I have attached a tentative fix and selftest to this patch, let me know what you think.
On 8/21/23 1:36 AM, Kumar Kartikeya Dwivedi wrote: > On Mon, 21 Aug 2023 at 05:33, Yonghong Song <yonghong.song@linux.dev> wrote: >> >> When reviewing local percpu kptr support, Alexei discovered a bug >> wherea bpf_kptr_xchg() may succeed even if the map value kptr type and >> locally allocated obj type do not match ([1]). Missed struct btf_id >> comparison is the reason for the bug. This patch added such struct btf_id >> comparison and will flag verification failure if types do not match. >> >> [1] https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t >> >> Reported-by: Alexei Starovoitov <ast@kernel.org> >> Fixes: 738c96d5e2e3 ("bpf: Allow local kptrs to be exchanged via bpf_kptr_xchg") >> Signed-off-by: Yonghong Song <yonghong.song@linux.dev> >> --- > > Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > But some comments below... > >> kernel/bpf/verifier.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index 02a021c524ab..4e1ecd4b8497 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -7745,7 +7745,18 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, >> verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n"); >> return -EFAULT; >> } >> - /* Handled by helper specific checks */ >> + if (meta->func_id == BPF_FUNC_kptr_xchg) { >> + struct btf_field *kptr_field = meta->kptr_field; >> + >> + if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off, >> + kptr_field->kptr.btf, kptr_field->kptr.btf_id, >> + true)) { >> + verbose(env, "R%d is of type %s but %s is expected\n", >> + regno, btf_type_name(reg->btf, reg->btf_id), >> + btf_type_name(kptr_field->kptr.btf, kptr_field->kptr.btf_id)); >> + return -EACCES; >> + } >> + } > > The fix on its own looks ok to me, but any reason you'd not like to > delegate to map_kptr_match_type? > Just to collect kptr related type matching logic in its own place. It > doesn't matter too much though. From comments from Alexei in https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t ===== The map_kptr_match_type() should have been used for kptrs pointing to kernel objects only. But you're calling it for MEM_ALLOC object with prog's BTF... ===== So looks like map_kptr_match_type() is for kptrs pointing to kernel objects only. So that is why I didn't use it. > > While looking at the code, I noticed one more problem. > > I don't think the current code is enforcing that 'reg->off is zero' > assumption when releasing MEM_ALLOC types. We are only saved because > you passed strict=true which makes passing non-zero reg->off a noop > and returns false. > The hunk was added to check_func_arg_reg_off in > 6a3cd3318ff6 ("bpf: Migrate release_on_unlock logic to non-owning ref > semantics") > which bypasses in case type is MEM_ALLOC and offset points to some > graph root or node. > > I am not sure why this check exists, IIUC rbtree release helpers are > not tagged KF_RELEASE, and no release helper can type match on them > either. Dave, can you confirm? I think it might be an accidental > leftover of some refactoring. I am sure that Dave will look into this problem. I will take a look as well to be sure my local percpu kptr won't have issues with what you just discovered. Thanks! > > In fact, it seems bpf_obj_drop is already broken because reg->off > assumption is violated. > For node_data type: > > bpf_obj_drop(&res->data); > returns > R1 must have zero offset when passed to release func > No graph node or root found at R1 type:node_data off:8 > > but bpf_obj_drop(&res->node); > passes verifier. > > 15: (bf) r1 = r0 ; > R0_w=ptr_node_data(ref_obj_id=3,off=16,imm=0) > R1_w=ptr_node_data(ref_obj_id=3,off=16,imm=0) refs=3 > 16: (b7) r2 = 0 ; R2_w=0 refs=3 > 17: (85) call bpf_obj_drop_impl#74867 ; > safe > > I have attached a tentative fix and selftest to this patch, let me > know what you think.
On Mon, 21 Aug 2023 at 19:52, Yonghong Song <yonghong.song@linux.dev> wrote: > > > > On 8/21/23 1:36 AM, Kumar Kartikeya Dwivedi wrote: > > On Mon, 21 Aug 2023 at 05:33, Yonghong Song <yonghong.song@linux.dev> wrote: > >> > >> When reviewing local percpu kptr support, Alexei discovered a bug > >> wherea bpf_kptr_xchg() may succeed even if the map value kptr type and > >> locally allocated obj type do not match ([1]). Missed struct btf_id > >> comparison is the reason for the bug. This patch added such struct btf_id > >> comparison and will flag verification failure if types do not match. > >> > >> [1] https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t > >> > >> Reported-by: Alexei Starovoitov <ast@kernel.org> > >> Fixes: 738c96d5e2e3 ("bpf: Allow local kptrs to be exchanged via bpf_kptr_xchg") > >> Signed-off-by: Yonghong Song <yonghong.song@linux.dev> > >> --- > > > > Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > > But some comments below... > > > >> kernel/bpf/verifier.c | 13 ++++++++++++- > >> 1 file changed, 12 insertions(+), 1 deletion(-) > >> > >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > >> index 02a021c524ab..4e1ecd4b8497 100644 > >> --- a/kernel/bpf/verifier.c > >> +++ b/kernel/bpf/verifier.c > >> @@ -7745,7 +7745,18 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > >> verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n"); > >> return -EFAULT; > >> } > >> - /* Handled by helper specific checks */ > >> + if (meta->func_id == BPF_FUNC_kptr_xchg) { > >> + struct btf_field *kptr_field = meta->kptr_field; > >> + > >> + if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off, > >> + kptr_field->kptr.btf, kptr_field->kptr.btf_id, > >> + true)) { > >> + verbose(env, "R%d is of type %s but %s is expected\n", > >> + regno, btf_type_name(reg->btf, reg->btf_id), > >> + btf_type_name(kptr_field->kptr.btf, kptr_field->kptr.btf_id)); > >> + return -EACCES; > >> + } > >> + } > > > > The fix on its own looks ok to me, but any reason you'd not like to > > delegate to map_kptr_match_type? > > Just to collect kptr related type matching logic in its own place. It > > doesn't matter too much though. > > From comments from Alexei in > > https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t > > ===== > The map_kptr_match_type() should have been used for kptrs pointing to > kernel objects only. > But you're calling it for MEM_ALLOC object with prog's BTF... > ===== > > So looks like map_kptr_match_type() is for kptrs pointing to > kernel objects only. So that is why I didn't use it. > That function was added by me. Back then I added this check as we were discussing possibly supporting such local kptr and more thought would be needed about the design before just doing type matching. Also it was using kernel_type_name which was later renamed as btf_type_name, so as a precaution I added the btf_is_kernel check. Apart from that I remember no other reason, so I think it should be ok to drop it now and use it. But as I said, it is up to you, it will in effect do the same thing as this patch, so it is ok as-is. > > > > While looking at the code, I noticed one more problem. > > > > I don't think the current code is enforcing that 'reg->off is zero' > > assumption when releasing MEM_ALLOC types. We are only saved because > > you passed strict=true which makes passing non-zero reg->off a noop > > and returns false. > > The hunk was added to check_func_arg_reg_off in > > 6a3cd3318ff6 ("bpf: Migrate release_on_unlock logic to non-owning ref > > semantics") > > which bypasses in case type is MEM_ALLOC and offset points to some > > graph root or node. > > > > I am not sure why this check exists, IIUC rbtree release helpers are > > not tagged KF_RELEASE, and no release helper can type match on them > > either. Dave, can you confirm? I think it might be an accidental > > leftover of some refactoring. > > I am sure that Dave will look into this problem. I will take > a look as well to be sure my local percpu kptr won't have > issues with what you just discovered. Thanks! > It should be a problem for anything that has the MEM_ALLOC flag set, including percpu_kptr.
On 8/21/23 9:03 AM, Kumar Kartikeya Dwivedi wrote: > On Mon, 21 Aug 2023 at 19:52, Yonghong Song <yonghong.song@linux.dev> wrote: >> >> >> >> On 8/21/23 1:36 AM, Kumar Kartikeya Dwivedi wrote: >>> On Mon, 21 Aug 2023 at 05:33, Yonghong Song <yonghong.song@linux.dev> wrote: >>>> >>>> When reviewing local percpu kptr support, Alexei discovered a bug >>>> wherea bpf_kptr_xchg() may succeed even if the map value kptr type and >>>> locally allocated obj type do not match ([1]). Missed struct btf_id >>>> comparison is the reason for the bug. This patch added such struct btf_id >>>> comparison and will flag verification failure if types do not match. >>>> >>>> [1] https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t >>>> >>>> Reported-by: Alexei Starovoitov <ast@kernel.org> >>>> Fixes: 738c96d5e2e3 ("bpf: Allow local kptrs to be exchanged via bpf_kptr_xchg") >>>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev> >>>> --- >>> >>> Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> >>> >>> But some comments below... >>> >>>> kernel/bpf/verifier.c | 13 ++++++++++++- >>>> 1 file changed, 12 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >>>> index 02a021c524ab..4e1ecd4b8497 100644 >>>> --- a/kernel/bpf/verifier.c >>>> +++ b/kernel/bpf/verifier.c >>>> @@ -7745,7 +7745,18 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, >>>> verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n"); >>>> return -EFAULT; >>>> } >>>> - /* Handled by helper specific checks */ >>>> + if (meta->func_id == BPF_FUNC_kptr_xchg) { >>>> + struct btf_field *kptr_field = meta->kptr_field; >>>> + >>>> + if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off, >>>> + kptr_field->kptr.btf, kptr_field->kptr.btf_id, >>>> + true)) { >>>> + verbose(env, "R%d is of type %s but %s is expected\n", >>>> + regno, btf_type_name(reg->btf, reg->btf_id), >>>> + btf_type_name(kptr_field->kptr.btf, kptr_field->kptr.btf_id)); >>>> + return -EACCES; >>>> + } >>>> + } >>> >>> The fix on its own looks ok to me, but any reason you'd not like to >>> delegate to map_kptr_match_type? >>> Just to collect kptr related type matching logic in its own place. It >>> doesn't matter too much though. >> >> From comments from Alexei in >> >> https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t >> >> ===== >> The map_kptr_match_type() should have been used for kptrs pointing to >> kernel objects only. >> But you're calling it for MEM_ALLOC object with prog's BTF... >> ===== >> >> So looks like map_kptr_match_type() is for kptrs pointing to >> kernel objects only. So that is why I didn't use it. >> > > That function was added by me. Back then I added this check as we were > discussing possibly supporting such local kptr and more thought would > be needed about the design before just doing type matching. Also it > was using kernel_type_name which was later renamed as btf_type_name, > so as a precaution I added the btf_is_kernel check. Apart from that I > remember no other reason, so I think it should be ok to drop it now > and use it. > > But as I said, it is up to you, it will in effect do the same thing as > this patch, so it is ok as-is. > >>> >>> While looking at the code, I noticed one more problem. >>> >>> I don't think the current code is enforcing that 'reg->off is zero' >>> assumption when releasing MEM_ALLOC types. We are only saved because >>> you passed strict=true which makes passing non-zero reg->off a noop >>> and returns false. >>> The hunk was added to check_func_arg_reg_off in >>> 6a3cd3318ff6 ("bpf: Migrate release_on_unlock logic to non-owning ref >>> semantics") >>> which bypasses in case type is MEM_ALLOC and offset points to some >>> graph root or node. >>> >>> I am not sure why this check exists, IIUC rbtree release helpers are >>> not tagged KF_RELEASE, and no release helper can type match on them >>> either. Dave, can you confirm? I think it might be an accidental >>> leftover of some refactoring. >> >> I am sure that Dave will look into this problem. I will take >> a look as well to be sure my local percpu kptr won't have >> issues with what you just discovered. Thanks! >> > > It should be a problem for anything that has the MEM_ALLOC flag set, > including percpu_kptr. I suspect that below removed code ``` diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 7fa46e92fe01..c0616c8b676d 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -7979,17 +7979,6 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, if (arg_type_is_dynptr(arg_type) && type == PTR_TO_STACK) return 0; - if ((type_is_ptr_alloc_obj(type) || type_is_non_owning_ref(type)) && reg->off) { - if (reg_find_field_offset(reg, reg->off, BPF_GRAPH_NODE_OR_ROOT)) - return __check_ptr_off_reg(env, reg, regno, true); - - verbose(env, "R%d must have zero offset when passed to release func\n", - regno); - verbose(env, "No graph node or root found at R%d type:%s off:%d\n", regno, - btf_type_name(reg->btf, reg->btf_id), reg->off); - return -EINVAL; - } - /* Doing check_ptr_off_reg check for the offset will catch this * because fixed_off_ok is false, but checking here allows us * to give the user a better error message. ``` intends to check whether there is a graph node or root in a local kptr. This is to ensure Dave's use case where you can locally allocate a obj and one of obj's field is a graph node/root. I agree with you the checking probably should not be here and it should be somewhere else.
On Mon, Aug 21, 2023 at 9:03 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > The fix on its own looks ok to me, but any reason you'd not like to > > > delegate to map_kptr_match_type? > > > Just to collect kptr related type matching logic in its own place. It > > > doesn't matter too much though. > > > > From comments from Alexei in > > > > https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t > > > > ===== > > The map_kptr_match_type() should have been used for kptrs pointing to > > kernel objects only. > > But you're calling it for MEM_ALLOC object with prog's BTF... > > ===== > > > > So looks like map_kptr_match_type() is for kptrs pointing to > > kernel objects only. So that is why I didn't use it. > > > > That function was added by me. Back then I added this check as we were > discussing possibly supporting such local kptr and more thought would > be needed about the design before just doing type matching. Also it > was using kernel_type_name which was later renamed as btf_type_name, > so as a precaution I added the btf_is_kernel check. Apart from that I > remember no other reason, so I think it should be ok to drop it now > and use it. Agree with Kumar. When I said map_kptr_match_type() is only used with kernel BTF I meant that as code stands it was the intent of that helper. With MEM_ALLOC being kptr_xchg-ed it's better to share the code and map_kptr_match_type() should probably be adopted to work with both kernel and prog's BTFs. And as Kumar noticed __check_ptr_off_reg() part of it is necessary for MEM_ALLOC too.
On 8/21/23 12:44 PM, Alexei Starovoitov wrote: > On Mon, Aug 21, 2023 at 9:03 AM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: >>>> >>>> The fix on its own looks ok to me, but any reason you'd not like to >>>> delegate to map_kptr_match_type? >>>> Just to collect kptr related type matching logic in its own place. It >>>> doesn't matter too much though. >>> >>> From comments from Alexei in >>> >>> https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t >>> >>> ===== >>> The map_kptr_match_type() should have been used for kptrs pointing to >>> kernel objects only. >>> But you're calling it for MEM_ALLOC object with prog's BTF... >>> ===== >>> >>> So looks like map_kptr_match_type() is for kptrs pointing to >>> kernel objects only. So that is why I didn't use it. >>> >> >> That function was added by me. Back then I added this check as we were >> discussing possibly supporting such local kptr and more thought would >> be needed about the design before just doing type matching. Also it >> was using kernel_type_name which was later renamed as btf_type_name, >> so as a precaution I added the btf_is_kernel check. Apart from that I >> remember no other reason, so I think it should be ok to drop it now >> and use it. > > Agree with Kumar. > When I said map_kptr_match_type() is only used with kernel BTF I meant > that as code stands it was the intent of that helper. > With MEM_ALLOC being kptr_xchg-ed it's better to share the code and > map_kptr_match_type() should probably be adopted to work with both kernel > and prog's BTFs. Sounds good to me. Will use map_kptr_match_type() in v2. > > And as Kumar noticed __check_ptr_off_reg() part of it is necessary for > MEM_ALLOC too.
On Mon, Aug 21, 2023 at 3:13 PM Yonghong Song <yonghong.song@linux.dev> wrote: > > > > On 8/21/23 12:44 PM, Alexei Starovoitov wrote: > > On Mon, Aug 21, 2023 at 9:03 AM Kumar Kartikeya Dwivedi > > <memxor@gmail.com> wrote: > >>>> > >>>> The fix on its own looks ok to me, but any reason you'd not like to > >>>> delegate to map_kptr_match_type? > >>>> Just to collect kptr related type matching logic in its own place. It > >>>> doesn't matter too much though. > >>> > >>> From comments from Alexei in > >>> > >>> https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t > >>> > >>> ===== > >>> The map_kptr_match_type() should have been used for kptrs pointing to > >>> kernel objects only. > >>> But you're calling it for MEM_ALLOC object with prog's BTF... > >>> ===== > >>> > >>> So looks like map_kptr_match_type() is for kptrs pointing to > >>> kernel objects only. So that is why I didn't use it. > >>> > >> > >> That function was added by me. Back then I added this check as we were > >> discussing possibly supporting such local kptr and more thought would > >> be needed about the design before just doing type matching. Also it > >> was using kernel_type_name which was later renamed as btf_type_name, > >> so as a precaution I added the btf_is_kernel check. Apart from that I > >> remember no other reason, so I think it should be ok to drop it now > >> and use it. > > > > Agree with Kumar. > > When I said map_kptr_match_type() is only used with kernel BTF I meant > > that as code stands it was the intent of that helper. > > With MEM_ALLOC being kptr_xchg-ed it's better to share the code and > > map_kptr_match_type() should probably be adopted to work with both kernel > > and prog's BTFs. > > Sounds good to me. Will use map_kptr_match_type() in v2. btw it's a bit risky for bpf tree this late into rc-s. Pls target bpf-next.
On 8/21/23 4:36 AM, Kumar Kartikeya Dwivedi wrote: > On Mon, 21 Aug 2023 at 05:33, Yonghong Song <yonghong.song@linux.dev> wrote: >> >> When reviewing local percpu kptr support, Alexei discovered a bug >> wherea bpf_kptr_xchg() may succeed even if the map value kptr type and >> locally allocated obj type do not match ([1]). Missed struct btf_id >> comparison is the reason for the bug. This patch added such struct btf_id >> comparison and will flag verification failure if types do not match. >> >> [1] https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t >> >> Reported-by: Alexei Starovoitov <ast@kernel.org> >> Fixes: 738c96d5e2e3 ("bpf: Allow local kptrs to be exchanged via bpf_kptr_xchg") >> Signed-off-by: Yonghong Song <yonghong.song@linux.dev> >> --- > > Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > But some comments below... > >> kernel/bpf/verifier.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index 02a021c524ab..4e1ecd4b8497 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -7745,7 +7745,18 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, >> verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n"); >> return -EFAULT; >> } >> - /* Handled by helper specific checks */ >> + if (meta->func_id == BPF_FUNC_kptr_xchg) { >> + struct btf_field *kptr_field = meta->kptr_field; >> + >> + if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off, >> + kptr_field->kptr.btf, kptr_field->kptr.btf_id, >> + true)) { >> + verbose(env, "R%d is of type %s but %s is expected\n", >> + regno, btf_type_name(reg->btf, reg->btf_id), >> + btf_type_name(kptr_field->kptr.btf, kptr_field->kptr.btf_id)); >> + return -EACCES; >> + } >> + } > > The fix on its own looks ok to me, but any reason you'd not like to > delegate to map_kptr_match_type? > Just to collect kptr related type matching logic in its own place. It > doesn't matter too much though. > > While looking at the code, I noticed one more problem. > > I don't think the current code is enforcing that 'reg->off is zero' > assumption when releasing MEM_ALLOC types. We are only saved because > you passed strict=true which makes passing non-zero reg->off a noop > and returns false. > The hunk was added to check_func_arg_reg_off in > 6a3cd3318ff6 ("bpf: Migrate release_on_unlock logic to non-owning ref > semantics") > which bypasses in case type is MEM_ALLOC and offset points to some > graph root or node. > > I am not sure why this check exists, IIUC rbtree release helpers are > not tagged KF_RELEASE, and no release helper can type match on them > either. Dave, can you confirm? I think it might be an accidental > leftover of some refactoring. I think you're correct, that's probably leftover from when helpers were tagged KF_RELEASE in earlier rbtree impl revisions. I also think it's reasonable to delete the hunk as you've done in attached patches. > > In fact, it seems bpf_obj_drop is already broken because reg->off > assumption is violated. > For node_data type: > > bpf_obj_drop(&res->data); > returns > R1 must have zero offset when passed to release func > No graph node or root found at R1 type:node_data off:8 > > but bpf_obj_drop(&res->node); > passes verifier. > > 15: (bf) r1 = r0 ; > R0_w=ptr_node_data(ref_obj_id=3,off=16,imm=0) > R1_w=ptr_node_data(ref_obj_id=3,off=16,imm=0) refs=3 > 16: (b7) r2 = 0 ; R2_w=0 refs=3 > 17: (85) call bpf_obj_drop_impl#74867 ; > safe > > I have attached a tentative fix and selftest to this patch, let me > know what you think.
On Tue, 22 Aug 2023 at 10:45, David Marchevsky <david.marchevsky@linux.dev> wrote: > > On 8/21/23 4:36 AM, Kumar Kartikeya Dwivedi wrote: > > On Mon, 21 Aug 2023 at 05:33, Yonghong Song <yonghong.song@linux.dev> wrote: > >> > >> When reviewing local percpu kptr support, Alexei discovered a bug > >> wherea bpf_kptr_xchg() may succeed even if the map value kptr type and > >> locally allocated obj type do not match ([1]). Missed struct btf_id > >> comparison is the reason for the bug. This patch added such struct btf_id > >> comparison and will flag verification failure if types do not match. > >> > >> [1] https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t > >> > >> Reported-by: Alexei Starovoitov <ast@kernel.org> > >> Fixes: 738c96d5e2e3 ("bpf: Allow local kptrs to be exchanged via bpf_kptr_xchg") > >> Signed-off-by: Yonghong Song <yonghong.song@linux.dev> > >> --- > > > > Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > > But some comments below... > > > >> kernel/bpf/verifier.c | 13 ++++++++++++- > >> 1 file changed, 12 insertions(+), 1 deletion(-) > >> > >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > >> index 02a021c524ab..4e1ecd4b8497 100644 > >> --- a/kernel/bpf/verifier.c > >> +++ b/kernel/bpf/verifier.c > >> @@ -7745,7 +7745,18 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > >> verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n"); > >> return -EFAULT; > >> } > >> - /* Handled by helper specific checks */ > >> + if (meta->func_id == BPF_FUNC_kptr_xchg) { > >> + struct btf_field *kptr_field = meta->kptr_field; > >> + > >> + if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off, > >> + kptr_field->kptr.btf, kptr_field->kptr.btf_id, > >> + true)) { > >> + verbose(env, "R%d is of type %s but %s is expected\n", > >> + regno, btf_type_name(reg->btf, reg->btf_id), > >> + btf_type_name(kptr_field->kptr.btf, kptr_field->kptr.btf_id)); > >> + return -EACCES; > >> + } > >> + } > > > > The fix on its own looks ok to me, but any reason you'd not like to > > delegate to map_kptr_match_type? > > Just to collect kptr related type matching logic in its own place. It > > doesn't matter too much though. > > > > While looking at the code, I noticed one more problem. > > > > I don't think the current code is enforcing that 'reg->off is zero' > > assumption when releasing MEM_ALLOC types. We are only saved because > > you passed strict=true which makes passing non-zero reg->off a noop > > and returns false. > > The hunk was added to check_func_arg_reg_off in > > 6a3cd3318ff6 ("bpf: Migrate release_on_unlock logic to non-owning ref > > semantics") > > which bypasses in case type is MEM_ALLOC and offset points to some > > graph root or node. > > > > I am not sure why this check exists, IIUC rbtree release helpers are > > not tagged KF_RELEASE, and no release helper can type match on them > > either. Dave, can you confirm? I think it might be an accidental > > leftover of some refactoring. > > I think you're correct, that's probably leftover from when > helpers were tagged KF_RELEASE in earlier rbtree impl revisions. > > I also think it's reasonable to delete the hunk as you've done > in attached patches. > Ok, then I'll submit it as a patch after Yonghong's patch is accepted (since both will conflict trying to add local_kptr_stash_fail test cases) and you can give your Acked-by on it.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 02a021c524ab..4e1ecd4b8497 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -7745,7 +7745,18 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n"); return -EFAULT; } - /* Handled by helper specific checks */ + if (meta->func_id == BPF_FUNC_kptr_xchg) { + struct btf_field *kptr_field = meta->kptr_field; + + if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off, + kptr_field->kptr.btf, kptr_field->kptr.btf_id, + true)) { + verbose(env, "R%d is of type %s but %s is expected\n", + regno, btf_type_name(reg->btf, reg->btf_id), + btf_type_name(kptr_field->kptr.btf, kptr_field->kptr.btf_id)); + return -EACCES; + } + } break; case PTR_TO_BTF_ID | MEM_PERCPU: case PTR_TO_BTF_ID | MEM_PERCPU | PTR_TRUSTED:
When reviewing local percpu kptr support, Alexei discovered a bug wherea bpf_kptr_xchg() may succeed even if the map value kptr type and locally allocated obj type do not match ([1]). Missed struct btf_id comparison is the reason for the bug. This patch added such struct btf_id comparison and will flag verification failure if types do not match. [1] https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t Reported-by: Alexei Starovoitov <ast@kernel.org> Fixes: 738c96d5e2e3 ("bpf: Allow local kptrs to be exchanged via bpf_kptr_xchg") Signed-off-by: Yonghong Song <yonghong.song@linux.dev> --- kernel/bpf/verifier.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)