Re: [PATCH RFC v1 09/15] xdp: clear grow memory in bpf_xdp_adjust_tail()

On Wed, 18 Mar 2020 10:15:38 +0100
Toke Høiland-Jørgensen [off-list ref] wrote:

Jesper Dangaard Brouer [off-list ref] writes:

quoted

To reviewers: Need some opinions if this is needed?

(TODO: Squash patch)
---
 net/core/filter.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index 0ceddee0c678..669f29992177 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c

@@ -3432,6 +3432,12 @@ BPF_CALL_2(bpf_xdp_adjust_tail, struct xdp_buff *, xdp, int, offset)
 	if (unlikely(data_end < xdp->data + ETH_HLEN))
 		return -EINVAL;
 
+	// XXX: To reviewers: How paranoid are we? Do we really need to
+	/* clear memory area on grow, as in-theory can contain uninit kmem */
+	if (offset > 0) {
+		memset(xdp->data_end, 0, offset);
+	}  

This memory will usually be recycled through page_pool or equivalent,
right? So couldn't we clear the pages when they are first allocated?
That way, the only data that would be left there would be packet data
from previous packets...

Yes, that is another option, to clear pages on "real" alloc (not
recycle alloc), but it is a bit harder to implement (when not using
page_pool).

And yes, this area will very likely just contain old packet data, but
we cannot be 100% sure.

Previously Alexei have argued that we should not leak pointer values in
XDP.  Which is why we have xdp_scrub_frame(), but this is not 100% the
same.  So, I would like to hear Alexei's opinion ?

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer