On 9/12/24 7:59 PM, Andrii Nakryiko wrote:
On Thu, Sep 12, 2024 at 2:47 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
On 9/9/24 2:16 PM, Daniel Borkmann wrote:
On 9/7/24 12:35 AM, Alexei Starovoitov wrote:
On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
- if (unlikely(flags & ~(BPF_MTU_CHK_SEGS)))
- return -EINVAL;
-
- if (unlikely(flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))
+ if (unlikely((flags & ~(BPF_MTU_CHK_SEGS)) ||
+ (flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))) {
+ *mtu_len = 0;
return -EINVAL;
+ }
dev = __dev_via_ifindex(dev, ifindex);
- if (unlikely(!dev))
+ if (unlikely(!dev)) {
+ *mtu_len = 0;
return -ENODEV;
+ }
I don't understand this mtu_len clearing.
My earlier comment was that mtu is in&out argument.
The program has to set it to something. It cannot be uninit.
So zeroing it on error looks very odd.
In that sense the patch 3 looks wrong. Instead of:
@@ -6346,7 +6346,9 @@ static const struct bpf_func_proto
bpf_skb_check_mtu_proto = {
.ret_type = RET_INTEGER,
.arg1_type = ARG_PTR_TO_CTX,
.arg2_type = ARG_ANYTHING,
- .arg3_type = ARG_PTR_TO_INT,
+ .arg3_type = ARG_PTR_TO_FIXED_SIZE_MEM |
+ MEM_UNINIT | MEM_ALIGNED,
+ .arg3_size = sizeof(u32),
MEM_UNINIT should be removed, because
bpf_xdp_check_mtu, bpf_skb_check_mtu will read it.
If there is a program out there that calls this helper without
initializing mtu_len it will be rejected after we fix it,
but I think that's a good thing.
Passing random mtu_len and let helper do:
skb_len = *mtu_len ? *mtu_len + dev->hard_header_len : skb->len;
is just bad.
Ok, fair. Removing MEM_UNINIT sounds reasonable, was mostly thinking
that even if its garbage MTU being passed in, mtu_len gets filled in
either case (BPF_MTU_CHK_RET_{SUCCESS,FRAG_NEEDED}) assuming no invalid
ifindex e.g.:
__u32 mtu_len;
bpf_skb_check_mtu(skb, skb->ifindex, &mtu_len, 0, 0);
Getting back at this, removing MEM_UNINIT needs more verifier rework:
MEM_UNINIT right now implies two things actually: i) write into memory,
ii) memory does not have to be initialized. If we lift MEM_UNINIT, it
then becomes: i) read into memory, ii) memory must be initialized.
This means that for bpf_*_check_mtu() we're readding the issue we're
trying to fix, that is, it would then be able to write back into things
like .rodata maps.
My suggestion is for this series is to go with MEM_UNINIT tag and
clearing on error case to avoid leaking, and then in a subsequent
series to break up MEM_UNINIT in the verifier into two properties:
MEM_WRITE and MEM_UNINIT to better declare intent of the helpers. Then
the bpf_*_check_mtu() will have MEM_WRITE but not MEM_UNINIT.
Thoughts? (If preference is to further extend this series, I can also
look into that ofc.)
We have MEM_RDONLY which literally means that memory is meant to be
only consumed for reading. Will this solve these issues?
It doesn't help, in check_helper_mem_access() we check for writes e.g.
for map values only via BPF_WRITE when meta->raw_mode is set which is
the case for MEM_UNINIT-tagged ARG_PTR_TO_MEM, otherwise BPF_READ is
implied hence my suggestion to break this up into two properties as a
next step.
Thanks,
Daniel
