Re: [PATCH v3 net-next RFC] Generic XDP

[Johannes Berg <johannes@sipsolutions.net>]

On Thu, 2017-04-13 at 16:01 -0400, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Thu, 13 Apr 2017 21:22:21 +0200
> 
> > OTOH, it might depend on the frame data itself, if the program does
> > something like
> > 
> > xdp->data[xdp->data[0] & 0xf]
> > 
> > (read or write, doesn't really matter) so then the verifier would
> have
> > to take the maximum possible value there into account.
> 
> I am not well versed enough with the verifier to understand exactly
> how and to what extent SKB accesses are validated by the verifier.
> 
> My, perhaps mistaken, impression is that access range validation is
> still at least partially done at run time.

I think you're right for SKB accesses, but I'm pretty sure that for XDP
the verifier checks that the program can't possibly access outside of
[xdp->data, xdp->data_end], see compare_ptrs_to_packet().

This checks that comparisons to data_end are all there, i.e. that the
program verifies it may access some bytes before actually doing so.
However, the program could start with

if (xdp->data_end < xdp->data + 1024)
	return DROP;
[...]

and then the verifier would consider it safe.

Still, it has to track down into the [...] code to actually understand
that it now didn't try to access xdp->data+1025, and as such it should
be able to determine the maximum desired offset.

However, I'm coming to realize that may not necessarily mean that the
program really *needs* to access that data.

For example, a hypothetical wifi program might want to recalculate and
compare the CRC checksum (for whatever reason, say a driver/hardware
bug). This would require accessing the last 4 bytes of the packet,
which may not be present. The program could, however, accept that
sometimes this isn't possible, and simply accept frames when it can't
see the last 4 bytes (or if the last 4 bytes aren't the CRC because
that isn't reported now, but whatever, I'm handwaving anyway.)

So perhaps this isn't really a good idea. The program should probably
separately say how much data it really *needs* there, and then perhaps
a warning could be emitted if it never accesses the data that it
advertises as needing (i.e. if it says "I want 1024 bytes" but then
can't possibly read more than 14)

johannes