lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 9 Sep 2022 16:12:31 -0700
From:   Martin KaFai Lau <>
To:     Joanne Koong <>
Subject: Re: [PATCH bpf-next v6 1/3] bpf: Add skb dynptrs

On 9/7/22 11:31 AM, Joanne Koong wrote:
> For bpf prog types that don't support writes on skb data, the dynptr is
> read-only (bpf_dynptr_write() will return an error and bpf_dynptr_data()
> will return NULL; for a read-only data slice, there will be a separate
> API bpf_dynptr_data_rdonly(), which will be added in the near future).
I just caught up on the v4 discussion about loadtime-vs-runtime error on 
write.  From a user perspective, I am not concerned on which error. 
Either way, I will quickly find out the packet header is not changed.

For the dynptr init helper bpf_dynptr_from_skb(), the user does not need 
to know its skb is read-only or not and uses the same helper.  The 
verifier in this case uses its knowledge on the skb context and uses 
bpf_dynptr_from_skb_rdonly_proto or bpf_dynptr_from_skb_rdwr_proto 

Now for the slice helper, the user needs to remember its skb is read 
only (or not) and uses bpf_dynptr_data() vs bpf_dynptr_data_rdonly() 
accordingly.  Yes, if it only needs to read, the user can always stay 
with bpf_dynptr_data_rdonly (which is not the initially supported one 
though).  However, it is still unnecessary burden and surprise to user. 
It is likely it will silently turn everything into bpf_dynptr_read() 
against the user intention. eg:

if (bpf_dynptr_from_skb(skb, 0, &dynptr))
	return 0;
ip6h = bpf_dynptr_data(&dynptr, 0, sizeof(*ip6h));
if (!ip6h) {
	/* Unlikely case, in non-linear section, just bpf_dynptr_read()
	 * Oops...actually bpf_dynptr_data_rdonly() should be used.
	bpf_dynptr_read(buf, sizeof(*ip6h), &dynptr, 0, 0);
	ip6h = buf;

> +	{
> +		struct sk_buff *skb = ptr->data;
> +
> +		/* if the data is paged, the caller needs to pull it first */
> +		if (ptr->offset + offset + len > skb->len - skb->data_len)

nit. skb_headlen(skb)

The patches can't be applied cleanly also. Please remember to rebase. 
eg. commit afef88e65554 ("selftests/bpf: Store BPF object files with 
.bpf.o extension") has landed on Sep 2.

Powered by blists - more mailing lists