This bug was fixed through a patchset containing 9 patches. Patches 7 and 8 actually fix the bug.

Patchset:
https://lore.kernel.org/bpf/20211217003152.48334-1-haoluo@google.com/

Upstream commits:
[1/9] d639b9d13a39cf15639cbe6e8b2c43eb60148a73 bpf: Introduce composable reg, ret and arg types.
[2/9] 48946bd6a5d695c50b34546864b79c1f910a33c1 bpf: Replace ARG_XXX_OR_NULL with ARG_XXX | PTR_MAYBE_NULL
[3/9] 3c4807322660d4290ac9062c034aed6b87243861 bpf: Replace RET_XXX_OR_NULL with RET_XXX | PTR_MAYBE_NULL
[4/9] c25b2ae136039ffa820c26138ed4a5e5f3ab3841 bpf: Replace PTR_TO_XXX_OR_NULL with PTR_TO_XXX | PTR_MAYBE_NULL
[5/9] 20b2aff4bc15bda809f994761d5719827d66c0b4 bpf: Introduce MEM_RDONLY flag
[6/9] cf9f2f8d62eca810afbd1ee6cc0800202b000e57 bpf: Convert PTR_TO_MEM_OR_NULL to composable types.
[7/9] 34d3a78c681e8e7844b43d1a2f4671a04249c821 bpf: Make per_cpu_ptr return rdonly PTR_TO_MEM.
[8/9] 216e3cd2f28dbbf1fe86848e0e29e6693b9f0a20 bpf: Add MEM_RDONLY for helper args that are pointers to rdonly mem.
[9/9] 9497c458c10b049438ef6e6ddda898edbc3ec6a8 bpf/selftests: Test PTR_TO_RDONLY_MEM

It seems that patch 7 is security-critical, while patch 8 is added for correctness. If that assumption is correct, the following are affected:
 - stable
 - SLE15-SP4-GA (BPF_UNPRIV_DEFAULT_OFF enabled)

Already fixed in master.

I'll take this

(In reply to Carlos López from comment #1)
> [snip]
> It seems that patch 7 is security-critical, while patch 8 is added for
> correctness. If that assumption is correct, the following are affected:
>  - stable
>  - SLE15-SP4-GA (BPF_UNPRIV_DEFAULT_OFF enabled)
> 
> Already fixed in master.

Agree with the assessment.

stable should be getting the fix soon in 5.16.11[1]

1: https://lore.kernel.org/stable/20220221084935.121206549@linuxfoundation.org/

@Tony, this seems to be yet another bug that depends on composable type refactoring d639b9d13a39..cf9f2f8d62ec (mainly changing kernel/bpf/verifier.c) that does the following changes:

- ARG_PTR_TO_OR_NULL => PTR_MAYBE_NULL | ARG_PTR_TO
- RET_PTR_TO_OR_NULL => PTR_MAYBE_NULL | RET_PTR_TO
- PTR_TO_OR_NULL => PTR_MAYBE_NULL | PTR_TO
- PTR_TO_RDWR_BUF => PTR_TO_BUF
- PTR_TO_RDONLY_BUF => PTR_TO_BUF | MEM_RDONLY
- Turn the original reg_type_str[] array into a function

When I was backporting for bsc#1194111 (CVE-2021-4204) and bsc#1194765 (CVE-2022-23222) I decided to *omit* the composable type patches since I find them to be a bit too much, and replaced PTR_TO_BUF with PTR_TO_RDWR_BUF, reg_type_str(env, type) with reg_type_str[type], etc. However now I’m reconsidering the approach.

Functionality-wise composable type refactoring doesn’t add or remove any feature, and the changes are specific and clean. In addition, it will make future backporting much easier. So I think it’s worth pulling.

OTOH it’s also not that difficult to introduce new enums, e.g. PTR_TO_RDONLY_MEM instead of using PTR_TO_MEM | MEM_RDONLY, which doesn’t require composable type.

What do you think?

(In reply to Shung-Hsi Yu from comment #5)
> @Tony, this seems to be yet another bug that depends on composable type
> refactoring d639b9d13a39..cf9f2f8d62ec (mainly changing
> kernel/bpf/verifier.c) that does the following changes:
> 
> - ARG_PTR_TO_OR_NULL => PTR_MAYBE_NULL | ARG_PTR_TO
> - RET_PTR_TO_OR_NULL => PTR_MAYBE_NULL | RET_PTR_TO
> - PTR_TO_OR_NULL => PTR_MAYBE_NULL | PTR_TO
> - PTR_TO_RDWR_BUF => PTR_TO_BUF
> - PTR_TO_RDONLY_BUF => PTR_TO_BUF | MEM_RDONLY
> - Turn the original reg_type_str[] array into a function
> 
> [snip]

The above should be as follow:

- ARG_PTR_TO_*_OR_NULL => PTR_MAYBE_NULL | ARG_PTR_TO_*
- RET_PTR_TO_*_OR_NULL => PTR_MAYBE_NULL | RET_PTR_TO_*
- PTR_TO_*_OR_NULL => PTR_MAYBE_NULL | PTR_TO_*
- PTR_TO_RDWR_BUF => PTR_TO_BUF
- PTR_TO_RDONLY_BUF => PTR_TO_BUF | MEM_RDONLY
- Turn the original reg_type_str[] array into a function

The asterisk sign somehow got lost due to conversion of my markdown notes to plain text.

Testing the composable type backport right now (users/syu/SLE15-SP4/bpf-composable-types), seems to be quite a bit of kABI breakage due to changes on enum bpf_reg_type. Hmm...

(In reply to Shung-Hsi Yu from comment #7)
> Testing the composable type backport right now
> (users/syu/SLE15-SP4/bpf-composable-types), seems to be quite a bit of kABI
> breakage due to changes on enum bpf_reg_type. Hmm...

Right.  This is why I wasn't interested in taking it.

The following commits fixes the issue:

  34d3a78c681e bpf: Make per_cpu_ptr return rdonly PTR_TO_MEM.

The following commit add a test for the CVE:

  9497c458c10b bpf/selftests: Test PTR_TO_RDONLY_MEM

The following are dependency pulled in to aid backporting:

  d639b9d13a39 bpf: Introduce composable reg, ret and arg types.
  48946bd6a5d6 bpf: Replace ARG_XXX_OR_NULL with ARG_XXX | PTR_MAYBE_NULL
  3c4807322660 bpf: Replace RET_XXX_OR_NULL with RET_XXX | PTR_MAYBE_NULL
  c25b2ae13603 bpf: Replace PTR_TO_XXX_OR_NULL with PTR_TO_XXX | PTR_MAYBE_NULL
  20b2aff4bc15 bpf: Introduce MEM_RDONLY flag
  cf9f2f8d62ec bpf: Convert PTR_TO_MEM_OR_NULL to composable types.
  216e3cd2f28d bpf: Add MEM_RDONLY for helper args that are pointers to rdonly mem.

Reassigning back to security team

Verified the fix and submitted 4db4b9b518f1 to SLE15-SP4-GA

(In reply to Carlos López from comment #1)
> This bug was fixed through a patchset containing 9 patches. Patches 7 and 8
> actually fix the bug.
> 
> Patchset:

<snip>

> [7/9] 34d3a78c681e8e7844b43d1a2f4671a04249c821 bpf: Make per_cpu_ptr return
> rdonly PTR_TO_MEM.
> [8/9] 216e3cd2f28dbbf1fe86848e0e29e6693b9f0a20 bpf: Add MEM_RDONLY for
> helper args that are pointers to rdonly mem.

> It seems that patch 7 is security-critical, while patch 8 is added for
> correctness. If that assumption is correct, the following are affected:
>  - stable
>  - SLE15-SP4-GA (BPF_UNPRIV_DEFAULT_OFF enabled)
> 


There's a livepatching clone (bsc#1196811) and I'd like to double-check with you that supported kernels before 15-SP4 are indeed not affected before closing that one.

For the 7/9 patch it's obvious, as we don't have the bpf_*_cpu_ptr() helpers in SLE15-SP3. But for the 8/9 ("bpf: Add MEM_RDONLY for helper args that are pointers to rdonly mem") not so much.

With my limited understanding of the BPF subsystem, this patch prevents passing (the BPF equivalent of a) const void * pointer to a helper function expecting a void *, where the latter would then potentially write into it. I read through the 15-SP3 code and I could not find any measure which would prevent this from happening. However, the damage seems limited there as PTR_TO_RDONLY_BUF is only being used for the annotation of the ->key component for bpf_map_elem iterators. That is, I believe that BPF programs attached to such iterators could write into ->key, even though it's being considered as RDONLY.

Assuming this is correct, map element iterators can be further divided into arraymap iterators and hashmap iterators: only these two map types provide a ->iter_seq_info for their respective bpf_map_ops.

arraymap iterators maintain a shadow copy of the ->key and have a proper bounds check in place (c.f. bpf_array_map_seq_show() + bpf_array_map_seq_next()), so the the worst an attached BPF program could achieve is to change the order of iteration over the map's elements, i.e. to affect itself.

For hashmap iterators OTOH the ->key passed to the attached BPF prog really is a pointer to the internal value, c.f. __bpf_hash_map_seq_show(). So I think that a BPF program could potentially alter a hash map element's stored key value, which would effectively mean that the given element would then become undiscoverable, because it's still being referenced from the former hash bucket. As attaching a BPF prog to a map element iterator requires an open FD to the map in the first place, the damage is again confined.

Shung-Hsi, can you confirm the above is valid or do you have any additional remarks?

(In reply to Nicolai Stange from comment #19)
> [snip]
> There's a livepatching clone (bsc#1196811) and I'd like to double-check with you that supported kernels before 15-SP4 are indeed not affected before closing that one.
> 
> For the 7/9 patch it's obvious, as we don't have the `bpf_*_cpu_ptr()` helpers in SLE15-SP3. But for the 8/9 ("bpf: Add `MEM_RDONLY` for helper args that are pointers to rdonly mem") not so much.

I somehow overlook the 8th patch when considering previous. Thanks for looking into this.

> With my limited understanding of the BPF subsystem, this patch prevents passing (the BPF equivalent of a) const void * pointer to a helper function expecting a `void *`, where the latter would then potentially write into it.

Yes, that's correct and you've explained it much better than I can.

> I read through the 15-SP3 code and I could not find any measure which would prevent this from happening. However, the damage seems limited there as `PTR_TO_RDONLY_BUF` is only being used for the annotation of the `->key` component for bpf_map_elem iterators. That is, I believe that BPF programs attached to such iterators could write into `->key`, even though it's being considered as RDONLY.

Agree, while PTR_TO_PACKET , PTR_TO_PACKET_META , PTR_TO_STACK , PTR_TO_MAP_VALUE , PTR_TO_MEM , PTR_TO_RDONLY_BUF , and PTR_TO_RDWR_BUF are all compatible with ARG_PTR_TO_MEM. Only the content of PTR_TO_RDONLY_BUF is not meant to be altered.

But only PTR_TO_RDONLY_BUF is not supposed to be touched

> Assuming this is correct, map element iterators can be further divided into arraymap iterators and hashmap iterators: only these two map types provide a `->iter_seq_info` for their respective bpf_map_ops.
> 
> arraymap iterators maintain a shadow copy of the `->key` and have a proper bounds check in place (c.f. bpf_array_map_seq_show() + bpf_array_map_seq_next()), so the the worst an attached BPF program could achieve is to change the order of iteration over the map's elements, i.e. to affect itself.
> 
> For hashmap iterators OTOH the `->key` passed to the attached BPF prog really is a pointer to the internal value, c.f. `__bpf_hash_map_seq_show()`. So I think that a BPF program could potentially alter a hash map element's stored key value, which would effectively mean that the given element would then become undiscoverable, because it's still being referenced from the former hash bucket. As attaching a BPF prog to a map element iterator requires an open FD to the map in the first place, the damage is again confined.
> 
> Shung-Hsi, can you confirm the above is valid or do you have any additional remarks?

I agree the above is valid. Being able to alter ->key, while technically an incorrect behavior, does not seem to be exploitable.

And for the sake of completeness, I'd like to add that it's also not possible for to use pointer arithmetic to mislead helpers into writing into an out-of-bound location, bpf_iter_attach_map() check for the max access size, and a negative offset is not allowed.

closing