Re: using verifier to ensure a BPF program uses certain metadata?
On Wed, Oct 18, 2017 at 08:56:31AM +0200, Johannes Berg wrote:
> > > Now, I realize that people could trivially just work around this in
> > > their program if they wanted, but I think most will take the
> > > reminder
> > > and just implement
> > >
> > > if (ctx->is_data_ethernet)
> > > return DROP_FRAME;
> > >
> > > instead, since mostly data frames will not be very relevant to
> > > them.
> > >
> > > What do you think?
> >
> > sounds fine and considering new verifier ops after Jakub refactoring
> > a check that is_data_ethernet was accessed would fit nicely.
> > Without void** hack.
>
> Ok, thanks! I'll have to check what Jakub is doing there, do you have a
> pointer to that refactoring?
something similar to
commit 4f9218aaf8a4 ("bpf: move knowledge about post-translation offsets out of
verifier")
Re: using verifier to ensure a BPF program uses certain metadata?
Hi Alexei, > > https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next > > .git/log/?h=bpf > > bpf bits looks pretty straightforward. Thanks for looking at this! > attach looks fine too. I'm assuming there is some rtnl or other lock, > so multiple assigns cannot race? Yes. > It's missing query interface though. > Please add support to return prog_id. Good point, this is about half a year old, so ... :) [...] > > Now, I realize that people could trivially just work around this in > > their program if they wanted, but I think most will take the > > reminder > > and just implement > > > > if (ctx->is_data_ethernet) > > return DROP_FRAME; > > > > instead, since mostly data frames will not be very relevant to > > them. > > > > What do you think? > > sounds fine and considering new verifier ops after Jakub refactoring > a check that is_data_ethernet was accessed would fit nicely. > Without void** hack. Ok, thanks! I'll have to check what Jakub is doing there, do you have a pointer to that refactoring? johannes
Re: using verifier to ensure a BPF program uses certain metadata?
On Mon, Oct 16, 2017 at 09:38:44AM +0200, Johannes Berg wrote: > Hi, > > As we discussed in April already (it's really been that long...), I'd > wanted to allow using BPF to filter wireless monitor frames, to enable > new use cases and higher performance in monitoring. I have some code, > at > > https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git/log/?h=bpf bpf bits looks pretty straightforward. attach looks fine too. I'm assuming there is some rtnl or other lock, so multiple assigns cannot race? It's missing query interface though. Please add support to return prog_id. > which implements parts of this. It's still missing the TX status path > and perhaps associated metadata, but that part is easy. > > The bigger "problem" is that we're going to be adding support for > devices that have 802.11->Ethernet conversion already in hardware, and > in that case the notion that the filter program will get an 802.11 > header to look at is no longer right. > > Now, most likely for the actual in-service monitoring we'll actually > have to reconstitute the 802.11 header on the fly (in pure monitoring > where nothing else is active, we can just disable the conversion), but > the filtering shouldn't really be reliant on that, since that's not the > cheapest thing to do. > > The obvious idea around this is to add a metadata field (just a bit > really), something like "is_data_ethernet", saying that it was both a > data frame and is already converted to have an Ethernet header. > However, since these devices don't really exist yet for the vast > majority of people, I'm a bit afraid that we'll find later a lot of > code simply ignoring this field and looking at the "802.11" header, > which is then broken if it encounters an Ethernet header instead. > > Are there lies my question: If we added a new callback to > bpf_verifier_ops (e.g. "post_verifier_check"), to be called after the > normal verification, and also added a context argument to > "is_valid_access" (*), we could easily track that this new metadata > field is accessed, and reject programs that don't access it at all. > > Now, I realize that people could trivially just work around this in > their program if they wanted, but I think most will take the reminder > and just implement > > if (ctx->is_data_ethernet) > return DROP_FRAME; > > instead, since mostly data frames will not be very relevant to them. > > What do you think? sounds fine and considering new verifier ops after Jakub refactoring a check that is_data_ethernet was accessed would fit nicely. Without void** hack.
using verifier to ensure a BPF program uses certain metadata?
Hi, As we discussed in April already (it's really been that long...), I'd wanted to allow using BPF to filter wireless monitor frames, to enable new use cases and higher performance in monitoring. I have some code, at https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git/log/?h=bpf which implements parts of this. It's still missing the TX status path and perhaps associated metadata, but that part is easy. The bigger "problem" is that we're going to be adding support for devices that have 802.11->Ethernet conversion already in hardware, and in that case the notion that the filter program will get an 802.11 header to look at is no longer right. Now, most likely for the actual in-service monitoring we'll actually have to reconstitute the 802.11 header on the fly (in pure monitoring where nothing else is active, we can just disable the conversion), but the filtering shouldn't really be reliant on that, since that's not the cheapest thing to do. The obvious idea around this is to add a metadata field (just a bit really), something like "is_data_ethernet", saying that it was both a data frame and is already converted to have an Ethernet header. However, since these devices don't really exist yet for the vast majority of people, I'm a bit afraid that we'll find later a lot of code simply ignoring this field and looking at the "802.11" header, which is then broken if it encounters an Ethernet header instead. Are there lies my question: If we added a new callback to bpf_verifier_ops (e.g. "post_verifier_check"), to be called after the normal verification, and also added a context argument to "is_valid_access" (*), we could easily track that this new metadata field is accessed, and reject programs that don't access it at all. Now, I realize that people could trivially just work around this in their program if they wanted, but I think most will take the reminder and just implement if (ctx->is_data_ethernet) return DROP_FRAME; instead, since mostly data frames will not be very relevant to them. What do you think? johannes (*) the context argument could just be a void **, and is_valid_access can allocate memory if needed - in this case I'd probably just do something like "return *is_valid_access ?: NULL;" and return something like "(void *)1" for when the field in question was accessed, and then just check that in "post_verifier_check".
