Re: [PATCH bpf 1/2] bpf: Fix OOB in bpf_obj_memcpy for cgroup storage

[xulang]

>>> This is fixing the src side of the "copy_map_value_long(map, dst, src)".
>>> The src could also be from a skb? What is the value_size that the
>>> verifier is checking for bpf_map_update_elem?
>>
>> The value_size checked by verifier is exactly the size with which
>> the map is defined, i.e., not the size rounded up to 8-byte by kernel
>
>If the verifier ensures only 4-bytes, I am not sure if the helper should
>read 8-bytes.
>
>> As for bpf_map_update_elem->..->copy_map_value_long, 'src' couldn't be from
>> 'skb' which mismatches the expected ptr-type of 'bpf_map_update_elem',
>> I've tried codes like these:
>>
>> 1. bpf_map_update_elem(&lru_map, &key, skb, BPF_ANY);
>> 2. bpf_map_update_elem(&lru_map, &key, skb->sk, BPF_ANY);  // null checked
>> 3. bpf_map_update_elem(&lru_map, &key, skb->flow_keys, BPF_ANY);
>>
>> All these ptrs mismatch the expected ptr-type, which can be detected by the verifier.
>> The verifier complains with msg like 'R3 type=ctx expected=fp, pkt, pkt_meta, map_key,
>> map_value, mem, ringbuf_mem, buf, trusted_ptr'
>
>I meant the __sk_buff->data. Take a look at how skb->data can be used in
>the selftests. __sk_buff->data may not have readable bytes rounded up to
>8. Just one example that the src cannot always be fixed by allocating more.
>
>From looking at git history on pcpu_init_value, the issue should be
>introduced in commit d3bec0138bfb.

Apologies, I missed that point. The 'src' could indeed be from a 'skb'.
Here is my test code:

SEC("cgroup_skb/ingress")
int trigger_oob_skb(struct __sk_buff *skb)
{
	__u32 key = 0;
	void *data, *data_end;
	// try to adjust offset to trigger OOB
	int data_len, offset = 64;

	data = (void *)(long)skb->data;
	data_end = (void *)(long)skb->data_end;

	if (data + offset > (char *)data_end)
		return 1;

	data_len = (char *)data_end - (char *)data;
	bpf_printk("data: %lx, len: %d, skb len: %d, skb: %lx",
		(unsigned long)data, data_len, skb->len, (unsigned long)skb);
	// the verifier rejects  using data_end - 4, so data + offset - 4 used instead.
	bpf_map_update_elem(&lru_pcpu_map, &key, data + offset - 4, BPF_ANY);

	return 1;
}

I've tried for several days, to adjust 'offset' to match the 'data_end', but couldn't
reproduce this OOB since there seems to be always a 'tail room' after the 'data_end'.
I've checked how the buffer of a 'skb' is alloced, take driver 'e1000' for example,
it has many complicated round-up operations, in my test, when I send a UDP package
with payload size less than 210, the alloced buffer size of a skb seems to be always
448, I've also checked the validity of its KASAN shadow memory, it also says 448. when
I go on increasing the payload size(over 210), for example, 211, the buffer size grows
sharply, I haven't found out when the 'data_end' reaches the 'tail end' yet, different
NIC driver may behave differently on the 'tail room'?

So, for now, I'm not quite sure if the skb->data is affected by this OOB, but I will
keep on testing and digging into the skb code.

>The cover letter did not reach the bpf list and patchwork — something is
>wrong with the reply-to. In general, avoid reply-to for multiple
>patches. This makes the thread unmanageable. The revision number is also
>missing in the subject.
>
>Please read submitting-patches.rst before posting again, in particular
>"The canonical patch format" and "In-Reply-To" sections. Test it with
>your inbox first.

As for the patch format, I've reviewed the doc you mentioned, thanks for the correction, 
I will resend the patch series as v3 in a new thread if I make it clear that the skb->data
is not affected by this OOB, or, just make a new fix patch since the former patch can't
work once and for all. In my opinion, 'pcpu_init_value' shouldn't read 8 bytes if it has
no guarantee that the 'src' is rounded up to 8-bytes.