Hi

When using direct packet access I noticed that the verifier cannot
cary the packet length validation check across tail calls. This is
mainly a burden for L4 where L3 may require some more expensive logic
to handle variable length headers.

Any objections to add this?

Thomas

On 07/02/2016 11:29 PM, Thomas Graf wrote:

Hi

When using direct packet access I noticed that the verifier cannot
cary the packet length validation check across tail calls. This is
mainly a burden for L4 where L3 may require some more expensive logic
to handle variable length headers.

Yeah, checks cannot be carried over in two occasions: i) calling helpers
that change skb->data (and therefore prior checks become invalid) and
ii) tail calls. For tail calls the verifier doesn't know how such programs
will be used or shared (e.g. they could be part of one or multiple tail
call maps but at the same time attached somewhere directly) so they strictly
need to be validated and assumed as stand-alone at that point in time.

So, I think with carrying checks across tail calls you then mean that you
store some non constant start offset/immediate e.g. into skb->cb[0] from
the program that executes the tail call, and would like to do the 'skb->data +
skb->cb[0] < skb->data_end' check once in the new program to give the verifier
some context? Since non constant, this would currently fail with "cannot
add integer value with 0 upper zero bits to ptr_to_packet" since the
unknown value from cb[0] has no tracking (imm==0) of upper zero bits (that
prevents/checks for overflows wrt skb->data since we do skb->data +
skb->cb[0]). Meaning we cannot safely perform such test against skb->data_end
before doing anything further.

So I think this might then require either some context field with similar
semantics as skb->data that you could set safely (since can be verified)
via a new helper function from the main program that does the tail call, or
you could perhaps advance skb->data from ctx to a save offset, so the tail
called program thinks it's as original skb->data then, but this would disallow
access before skb->data obviously. But also then you need to redo checks
when calling helpers that change skb's data since we potentially can call
into pskb_expand_head() and thus pointers to skb->data/data_end can change.
Do you need r+w case, right?

Any objections to add this?

Thomas

2016-07-04 13:02 GMT+02:00 Daniel Borkmann <daniel@...>:

Yeah, checks cannot be carried over in two occasions: i) calling helpers
that change skb->data (and therefore prior checks become invalid) and
ii) tail calls. For tail calls the verifier doesn't know how such programs
will be used or shared (e.g. they could be part of one or multiple tail
call maps but at the same time attached somewhere directly) so they strictly
need to be validated and assumed as stand-alone at that point in time.

So, I think with carrying checks across tail calls you then mean that you
store some non constant start offset/immediate e.g. into skb->cb[0] from
the program that executes the tail call, and would like to do the 'skb->data
+
skb->cb[0] < skb->data_end' check once in the new program to give the
verifier
some context? Since non constant, this would currently fail with "cannot
add integer value with 0 upper zero bits to ptr_to_packet" since the
unknown value from cb[0] has no tracking (imm==0) of upper zero bits (that
prevents/checks for overflows wrt skb->data since we do skb->data +
skb->cb[0]). Meaning we cannot safely perform such test against
skb->data_end
before doing anything further.

Exactly. This is what I tried and it is what I would believe be an
elegant solution to calculate variable headers once and carry it to
subsequent programs.

So I think this might then require either some context field with similar
semantics as skb->data that you could set safely (since can be verified)
via a new helper function from the main program that does the tail call, or
you could perhaps advance skb->data from ctx to a save offset, so the tail
called program thinks it's as original skb->data then, but this would
disallow
access before skb->data obviously. But also then you need to redo checks
when calling helpers that change skb's data since we potentially can call
into pskb_expand_head() and thus pointers to skb->data/data_end can change.
Do you need r+w case, right?

I thought about the extend skb->data but it is of limited use in
practice as variable headers are usually at L4 and the final action is
usually a L2/L3 rewrite of some sort. I guess if this is the only
option then it is definitely better than what we have now.

On Mon, Jul 04, 2016 at 04:21:23PM +0200, Thomas Graf wrote:

2016-07-04 13:02 GMT+02:00 Daniel Borkmann <daniel@...>:

Yeah, checks cannot be carried over in two occasions: i) calling helpers
that change skb->data (and therefore prior checks become invalid) and
ii) tail calls. For tail calls the verifier doesn't know how such programs
will be used or shared (e.g. they could be part of one or multiple tail
call maps but at the same time attached somewhere directly) so they strictly
need to be validated and assumed as stand-alone at that point in time.

So, I think with carrying checks across tail calls you then mean that you
store some non constant start offset/immediate e.g. into skb->cb[0] from
the program that executes the tail call, and would like to do the 'skb->data
+
skb->cb[0] < skb->data_end' check once in the new program to give the
verifier
some context? Since non constant, this would currently fail with "cannot
add integer value with 0 upper zero bits to ptr_to_packet" since the
unknown value from cb[0] has no tracking (imm==0) of upper zero bits (that
prevents/checks for overflows wrt skb->data since we do skb->data +
skb->cb[0]). Meaning we cannot safely perform such test against
skb->data_end
before doing anything further.

Exactly. This is what I tried and it is what I would believe be an
elegant solution to calculate variable headers once and carry it to
subsequent programs.

that would mean that we'd need to teach verifier to carry the register types
into cb[], so that subsequent prog can inherit them?
I'm not sure how that's possible yet.
Is length check is so expensive? The next prog will do it once.
Comparing to map lookup that's noise, no?

So I think this might then require either some context field with similar
semantics as skb->data that you could set safely (since can be verified)
via a new helper function from the main program that does the tail call, or
you could perhaps advance skb->data from ctx to a save offset, so the tail
called program thinks it's as original skb->data then, but this would
disallow
access before skb->data obviously. But also then you need to redo checks
when calling helpers that change skb's data since we potentially can call
into pskb_expand_head() and thus pointers to skb->data/data_end can change.
Do you need r+w case, right?

I thought about the extend skb->data but it is of limited use in
practice as variable headers are usually at L4 and the final action is
usually a L2/L3 rewrite of some sort. I guess if this is the only
option then it is definitely better than what we have now.

you mean something like tlv geneve like parsing? Why not to do
an optimisic const lenght check before parsing them?
Like assume there is always < 64 bytes of them and check for that once.