Hi,
On Mon, 16 May 2016 14:46:38 +0300, Dmitry V. Levin wrote:
> > Since the syscall is directly passed to qual_syscall() everything you used
> > before is still available (syscall number, name, classes...). The
> > occurrence is
> > actually an integer
>
> Any integer? Can it be negative or zero?
Nope, proper checks have to be added. Since this patch is more like a PoC there
is not much correctness checks as the user is still myself.
> > diff --git a/syscall.c b/syscall.c
> > index d71ead3..580660c 100644
> > --- a/syscall.c
> > +++ b/syscall.c
> > @@ -266,6 +266,8 @@ enum {
> > MIN_QUALS = MAX_NSYSCALLS > 255 ? MAX_NSYSCALLS : 255
> > };
> >
> > +struct_fail fails_vec[MAX_NSYSCALLS];
>
> Shouldn't there be not just one vector but SUPPORTED_PERSONALITIES
> vectors, like qual_vec?
Yes, I misunderstood why we needed the support for each personalities while
tracing a program.
> > + for (i = 0; i < nsyscall_vec[current_personality]; ++i)
> > + if (sysent_vec[current_personality][i].sys_name
> > + && !strcmp(ss, sysent_vec[current_personality][i].sys_name))
> > + break;
>
> Why limit this to current_personality?
For the same reason as above.
>
> > + if (i == nsyscall_vec[current_personality])
> > + goto bad_format;
> > + qual_syscall(ss, bitflag, not);
> > +
> > + ss = strtok_r(NULL, ":", &saveptr);
> > + if (!ss)
> > + goto bad_format;
> > +
> > + occ = strtol(ss, &end, 10);
> > + if (end == ss || (*end != '\0' && *end != '%') || errno == ERANGE)
> > + goto bad_format;
>
> What if occ is negative or zero?
As I said first it's a rough PoC, thanks for pointing this out though.
>
> > + fails_vec[i].occ = occ;
> > + if (*end == '%')
> > + fails_vec[i].cnt = -1;
> > +
> > + ss = strtok_r(NULL, ":", &saveptr);
> > + if (!ss)
> > + goto bad_format;
> > +
> > + if (*ss >= '0' && *ss <= '9') {
> > + err = strtol(ss, &end, 10);
> > + if (end == ss || *end != '\0' || errno == ERANGE)
> > + goto bad_format;
>
> What if err is negative or zero?
Same, thanks.
>
> > + }
> > + else {
> > + /* TODO: support string form. */
> > + }
> > + fails_vec[i].err = err;
> > + fails_vec[i].fail = 0;
> > +
> > + free(ms);
> > +
> > + return 0;
> > +
> > +bad_format:
> > + free(ms);
> > +
> > + return -1;
> > +}
>
> I suggest a simple change to this procedure: parse the string passed to
> the function and handle errors, then loop over personalities initializing
> qual_vec and fails_vec.
The only reason I see for duplicating code of qual_syscall() and qualify_one()
is a little speed improvement. Is that what you aim at?
>
> > +static int
> > qual_signal(const char *s, const unsigned int bitflag, const int not)
> > {
> > unsigned int i;
> > @@ -789,6 +851,35 @@ static void get_error(struct tcb *, const bool);
> > static int getregs_old(pid_t);
> > #endif
> >
> > +static long
> > +fail_syscall_enter(struct tcb *tcp)
> > +{
> > + if (fails_vec[tcp->scno].cnt != -1) {
> > + if (fails_vec[tcp->scno].cnt++ % fails_vec[tcp->scno].occ)
>
> What of fails_vec[tcp->scno].occ is zero?
Already discussed, thanks
>
> > + return 0;
> > + }
> > + else { /* TODO: Support the non deterministic way */
> > + return 0;
> > + }
> > +
> > + fails_vec[tcp->scno].fail = 1;
> > + return ptrace(PTRACE_POKEUSER, tcp->pid,
> > + offsetof(struct user, regs.orig_rax),
> > + (unsigned long)-1);
> > +}
>
> As this ptrace call is arch specific, I suggest moving it to a separate
> function, e.g. arch_set_scno.
Yes, definitely. This version was just a x86_64 PoC to ensure everything work
before diving into portability, error checking, testing...
>
> > +
> > +static inline long
> > +fail_syscall_exit(struct tcb *tcp)
> > +{
> > + if (!fails_vec[tcp->scno].fail)
> > + return 0;
> > +
> > + tcp->u_error = fails_vec[tcp->scno].err;
> > + return ptrace(PTRACE_POKEUSER, tcp->pid,
> > + offsetof(struct user, regs.rax),
> > + (unsigned long)-fails_vec[tcp->scno].err);
> > +}
>
> Likewise, I suggest moving arch specific code to a separate function,
> e.g. arch_set_error.
Sure, thanks.
>
> > +
> > static int
> > trace_syscall_entering(struct tcb *tcp)
> > {
> > @@ -842,6 +933,8 @@ trace_syscall_entering(struct tcb *tcp)
> > # endif
> > }
> > #endif
> > + if (tcp->qual_flg & QUAL_FAIL)
> > + fail_syscall_enter(tcp);
> >
> > if (!(tcp->qual_flg & QUAL_TRACE)
> > || (tracing_paths && !pathtrace_match(tcp))
> > @@ -908,6 +1001,9 @@ trace_syscall_exiting(struct tcb *tcp)
> > update_personality(tcp, tcp->currpers);
> > #endif
> > res = (get_regs_error ? -1 : get_syscall_result(tcp));
> > +
> > + if (tcp->qual_flg & QUAL_FAIL && fails_vec[tcp->scno].fail)
> > + fail_syscall_exit(tcp);
>
> The check for hide_log_until_execve should go first.
True, however, and correct me if I'm wrong but one can have both a filtered tcp
because syscall have not been selected for logging and a failing rule on that
syscall. That situation can be reflected as:
strace -e '!write' -e failwith=write:1:EINVAL /bin/ls
As in « don't output write syscalls since I already know they will fail ».
If we switch position the goto ret will occur and syscall will *not* be
discarded.
>
> It's important to make fail flag not per-scno but per-tcb, otherwise
> force-failing syscalls when tracing mulitple processes will be unreliable.
>
> struct_fail.fail is probably not needed at all.
> struct call_counts is better place for accounting purposes.
Yes struct_fail.fail should be replaced by a modulus check on call_counts.calls
I guess. However we may want the struct nevertheless to store information about
the error code and the fail frequency.
> > + if (tcp->qual_flg & QUAL_FAIL && fails_vec[tcp->scno].fail) {
> > + if ((unsigned int)u_error < nerrnos && errnoent[u_error])
> > + tprintf("= -1 %s ", errnoent[tcp->u_error]);
> > + else
> > + tprintf("= -1 %ld ", u_error);
> > + tprints("(DISCARDED)");
>
> Why DISCARDED?
It is just a simple way for one to know that this particular call failed on
purpose, hence it makes greping & dump parsing easier. Do you see any other
naming for that?
>
> > + fails_vec[tcp->scno].fail = 0;
> > + }
> > + else if (tcp->qual_flg & QUAL_RAW) {
> > if (u_error)
> > tprintf("= -1 (errno %ld)", u_error);
> > else
>
> What if both QUAL_FAIL and QUAL_RAW flags are set?
The case was not handled, thanks!
--
Nahim El Atmani <nahim+...@naam.me>
http://naam.me/
------------------------------------------------------------------------------
Mobile security can be enabling, not merely restricting. Employees who
bring their own devices (BYOD) to work are irked by the imposition of MDM
restrictions. Mobile Device Manager Plus allows you to control only the
apps on BYO-devices by containerizing them, leaving personal data untouched!
https://ad.doubleclick.net/ddm/clk/304595813;131938128;j
_______________________________________________
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel
