On 01/11/18 18:52, Arnaldo Carvalho de Melo wrote:
> R0=inv(id=0) R1=inv6 R2=inv6 R3=inv(id=0) R6=ctx(id=0,off=0,imm=0) R7=inv64
> R10=fp0,call_-1
> 15: (b7) r2 = 0
> 16: (63) *(u32 *)(r10 -260) = r2
> 17: (67) r1 <<= 32
> 18: (77) r1 >>= 32
> 19: (67) r1 <<= 3
> 20: (bf) r2 = r6
> 21: (0f) r2 += r1
> 22: (79) r3 = *(u64 *)(r2 +16)
> R2 invalid mem access 'inv'
I wonder if you could run this with verifier log level 2? (I'm not sure how
you would go about plumbing that through the perf tooling.) It seems very
odd that it ends up with R2=inv, and I'm wondering whether R1 becomes unknown
during the shifts or whether the addition in insn 21 somehow produces the
unknown-ness. (I know we used to have a thing[1] where doing ptr += K and
then also having an offset in the LDX produced an error about
ptr+const+const, but that seems to have been fixed at some point.)
Note however that even if we get past this, R1 at this point holds 6, so it
looks like the verifier is walking the impossible path where we're inside the
'if' even though filename_arg = 6. This is a (slightly annoying) verifier
limitation, that it walks paths with impossible combinations of constraints
(we've previously had cases where assertions in the verifier would blow up
because of this, e.g. registers with max_val less than min_val). So if the
check_ctx_access() is going to worry about whether you're off the end of the
array (I'm not sure what your program type is and thus which is_valid_access
callback is involved), then it'll complain about this.
If filename_arg came from some external source you'd have a different
problem, because then it would have a totally unknown value, that on entering
the 'if' becomes "unknown but < 6", which is still too variable to have as
the offset of a ctx access. Those have to be at a known constant offset, so
that we can determine the type of the returned value.
As a way to fix this, how about [UNTESTED!]:
const void *filename_arg = NULL;
/* ... */
switch (augmented_args.args.syscall_nr) {
case SYS_OPEN: filename_arg = args->args[0]; break;
case SYS_OPENAT: filename_arg = args->args[1]; break;
}
/* ... */
if (filename_arg) {
/* stuff */
blah = probe_read_str(/* ... */, filename_arg);
} else {
/* the other stuff */
}
That way, you're only ever dealing in constant pointers (although judging by
an old thread I found[1] about ptr+const+const, the compiler might decide to
make some optimisations that end up looking like your existing code).
As for what you want to do with the index coming from userspace, the verifier
will not like that at all, as mentioned above, so I think you'll need to do
something like:
switch (filename_arg_from_userspace) {
case 0: filename_arg = args->args[0]; break;
case 1: filename_arg = args->args[1]; break;
/* etc */
default: filename_arg = NULL;
}
thus ensuring that you only ever have ctx pointers with constant offsets.
-Ed
[1]: https://lists.iovisor.org/g/iovisor-dev/topic/21386327#1302
