On Wed, May 18, 2016 at 03:59:09AM +0200, Nahim El Atmani wrote: > 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.
No problem, I'd just reviewed it as if it was a final patch.
> > > 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;
> > > + 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;
> > > +
> > > + 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;
> > > + }
> > > + 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?
Is there any alternative way to initialize fails_vec for each personality and
syscalls involved? Maybe qualbits_t could be extended and fails_vec won't
be needed at all?
> > > 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.
If hide_log_until_execve is set, all syscalls are invoked by strace itself
and should not be tampered with.
The alternative is to set QUAL_FAIL flag in tcp->qual_flg only when the
decision to fail this particular syscall invocation has been made.
--
ldv
pgpkAGSHMhkVT.pgp
Description: PGP signature
------------------------------------------------------------------------------ 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
