| Message ID | 158446619342.702578.1522482431365026926.stgit@firesoul |
|---|---|
| State | RFC |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | XDP extend with knowledge of frame size | expand |
On Tue, 17 Mar 2020 18:29:53 +0100 Jesper Dangaard Brouer wrote: > To reviewers: Need some opinions if this is needed? > > (TODO: Squash patch) I'd vote we don't clear, since we don't clear in adjust head. We could also add some wrapper around memset() which could be compiled out based on some CONFIG_ but that could be seen as just moving the responsibility onto the user.. > 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); > + } > + > xdp->data_end = data_end; > > return 0; > >
Jesper Dangaard Brouer <brouer@redhat.com> writes: > 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... -Toke
On Wed, 18 Mar 2020 10:15:38 +0100 Toke Høiland-Jørgensen <toke@redhat.com> wrote: > Jesper Dangaard Brouer <brouer@redhat.com> writes: > > > 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 ?
On Wed, Mar 18, 2020 at 12:25 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote: > > On Wed, 18 Mar 2020 10:15:38 +0100 > Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > > Jesper Dangaard Brouer <brouer@redhat.com> writes: > > > > > 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 ? those were the days when sw didn't need to worry about hw bugs. Looks like these types of security issues are acceptable now, since pointer leaks no longer get CVE assigned.
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); + } + xdp->data_end = data_end; return 0;