From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43089) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Setg6-0003Za-UZ for qemu-devel@nongnu.org; Wed, 13 Jun 2012 15:56:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Setg4-0006hE-Pi for qemu-devel@nongnu.org; Wed, 13 Jun 2012 15:56:30 -0400 Received: from mail-yw0-f45.google.com ([209.85.213.45]:40927) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Setg4-0006h6-HI for qemu-devel@nongnu.org; Wed, 13 Jun 2012 15:56:28 -0400 Received: by yhoo21 with SMTP id o21so984613yho.4 for ; Wed, 13 Jun 2012 12:56:26 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: From: Blue Swirl Date: Wed, 13 Jun 2012 19:56:06 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC] [PATCHv2 2/2] Adding basic calls to libseccomp in vl.c List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Otubo Cc: qemu-devel@nongnu.org On Wed, Jun 13, 2012 at 7:20 PM, Eduardo Otubo w= rote: > I added a syscall struct using priority levels as described in the > libseccomp man page. The priority numbers are based to the frequency > they appear in a sample strace from a regular qemu guest run under > libvirt. > > Libseccomp generates linear BPF code to filter system calls, those rules > are read one after another. The priority system places the most common > rules first in order to reduce the overhead when processing them. > > Also, since this is just a first RFC, the whitelist is a little raw. We > might need your help to improve, test and fine tune the set of system > calls. > > v2: Fixed some style issues > =C2=A0 =C2=A0 =C2=A0 =C2=A0Removed code from vl.c and created qemu-seccom= p.[ch] > =C2=A0 =C2=A0 =C2=A0 =C2=A0Now using ARRAY_SIZE macro > =C2=A0 =C2=A0 =C2=A0 =C2=A0Added more syscalls without priority/frequency= set yet > > Signed-off-by: Eduardo Otubo > --- > =C2=A0qemu-seccomp.c | =C2=A0 73 ++++++++++++++++++++++++++++++++++++++++= ++++++++++++++++ > =C2=A0qemu-seccomp.h | =C2=A0 =C2=A09 +++++++ > =C2=A0vl.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =C2=A07 ++++++ > =C2=A03 files changed, 89 insertions(+) > =C2=A0create mode 100644 qemu-seccomp.c > =C2=A0create mode 100644 qemu-seccomp.h > > diff --git a/qemu-seccomp.c b/qemu-seccomp.c > new file mode 100644 > index 0000000..048b7ba > --- /dev/null > +++ b/qemu-seccomp.c > @@ -0,0 +1,73 @@ Copyright and license info missing. > +#include > +#include > +#include "qemu-seccomp.h" > + > +static struct QemuSeccompSyscall seccomp_whitelist[] =3D { 'const' > + =C2=A0 =C2=A0{ SCMP_SYS(timer_settime), 255 }, > + =C2=A0 =C2=A0{ SCMP_SYS(timer_gettime), 254 }, > + =C2=A0 =C2=A0{ SCMP_SYS(futex), 253 }, > + =C2=A0 =C2=A0{ SCMP_SYS(select), 252 }, > + =C2=A0 =C2=A0{ SCMP_SYS(recvfrom), 251 }, > + =C2=A0 =C2=A0{ SCMP_SYS(sendto), 250 }, > + =C2=A0 =C2=A0{ SCMP_SYS(read), 249 }, > + =C2=A0 =C2=A0{ SCMP_SYS(brk), 248 }, > + =C2=A0 =C2=A0{ SCMP_SYS(clone), 247 }, > + =C2=A0 =C2=A0{ SCMP_SYS(mmap), 247 }, > + =C2=A0 =C2=A0{ SCMP_SYS(mprotect), 246 }, > + =C2=A0 =C2=A0{ SCMP_SYS(ioctl), 245 }, > + =C2=A0 =C2=A0{ SCMP_SYS(recvmsg), 245 }, > + =C2=A0 =C2=A0{ SCMP_SYS(sendmsg), 245 }, > + =C2=A0 =C2=A0{ SCMP_SYS(accept), 245 }, > + =C2=A0 =C2=A0{ SCMP_SYS(connect), 245 }, > + =C2=A0 =C2=A0{ SCMP_SYS(bind), 245 }, It would be nice to avoid connect() and bind(). Perhaps seccomp init should be postponed to after all sockets have been created? > + =C2=A0 =C2=A0{ SCMP_SYS(listen), 245 }, > + =C2=A0 =C2=A0{ SCMP_SYS(ioctl), 245 }, > + =C2=A0 =C2=A0{ SCMP_SYS(eventfd), 245 }, > + =C2=A0 =C2=A0{ SCMP_SYS(rt_sigprocmask), 245 }, > + =C2=A0 =C2=A0{ SCMP_SYS(write), 244 }, > + =C2=A0 =C2=A0{ SCMP_SYS(fcntl), 243 }, > + =C2=A0 =C2=A0{ SCMP_SYS(tgkill), 242 }, > + =C2=A0 =C2=A0{ SCMP_SYS(rt_sigaction), 242 }, > + =C2=A0 =C2=A0{ SCMP_SYS(pipe2), 242 }, > + =C2=A0 =C2=A0{ SCMP_SYS(munmap), 242 }, > + =C2=A0 =C2=A0{ SCMP_SYS(mremap), 242 }, > + =C2=A0 =C2=A0{ SCMP_SYS(getsockname), 242 }, > + =C2=A0 =C2=A0{ SCMP_SYS(getpeername), 242 }, > + =C2=A0 =C2=A0{ SCMP_SYS(fdatasync), 242 }, > + =C2=A0 =C2=A0{ SCMP_SYS(close), 242 } > +}; > + > +#define seccomp_whitelist_count ARRAY_SIZE(seccomp_whitelist) I'm not sure the #define helps much. > + > +int seccomp_start(void) > +{ > + =C2=A0 =C2=A0int rc =3D 0; > + =C2=A0 =C2=A0unsigned int i =3D 0; > + > + =C2=A0 =C2=A0rc =3D seccomp_init(SCMP_ACT_KILL); > + =C2=A0 =C2=A0if (rc < 0) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0goto seccomp_return; > + =C2=A0 =C2=A0} > + > + =C2=A0 =C2=A0for (i =3D 0; i < seccomp_whitelist_count; i++) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0rc =3D seccomp_rule_add(SCMP_ACT_ALLOW, secc= omp_whitelist[i].num, 0); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (rc < 0) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto seccomp_return; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > + =C2=A0 =C2=A0 =C2=A0 =C2=A0rc =3D seccomp_syscall_priority(seccomp_whit= elist[i].num, > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0seccomp_white= list[i].priority); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (rc < 0) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto seccomp_return; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > + =C2=A0 =C2=A0} > + > + =C2=A0 =C2=A0rc =3D seccomp_load(); > + > + =C2=A0seccomp_return: > + =C2=A0 =C2=A0seccomp_release(); > + =C2=A0 =C2=A0if (rc < 0) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0fprintf(stderr, > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"ERROR: failed t= o configure the seccomp syscall filter in the kernel\n"); Should this be fatal? > + =C2=A0 =C2=A0} > + =C2=A0 =C2=A0return rc; Return value is not used. > +} > diff --git a/qemu-seccomp.h b/qemu-seccomp.h > new file mode 100644 > index 0000000..3bbdd87 > --- /dev/null > +++ b/qemu-seccomp.h > @@ -0,0 +1,9 @@ Usual header protection #ifndeffery missing. > +#include > +#include "osdep.h" > + > +struct QemuSeccompSyscall { > + =C2=A0 =C2=A0int32_t num; > + =C2=A0 =C2=A0uint8_t priority; > +}; This definition is not used elsewhere, so it should be internal to qemu-seccomp.c. > + > +int seccomp_start(void); > diff --git a/vl.c b/vl.c > index 204d85b..315afaf 100644 > --- a/vl.c > +++ b/vl.c > @@ -61,6 +61,9 @@ > > =C2=A0#include > =C2=A0#include > +#ifdef CONFIG_LIBSECCOMP > +#include "qemu-seccomp.h" > +#endif > =C2=A0#endif > =C2=A0#ifdef __sun__ > =C2=A0#include > @@ -2296,6 +2299,10 @@ int main(int argc, char **argv, char **envp) > =C2=A0 =C2=A0 const char *trace_events =3D NULL; > =C2=A0 =C2=A0 const char *trace_file =3D NULL; > > +#ifdef CONFIG_LIBSECCOMP > + =C2=A0 =C2=A0seccomp_start(); > +#endif > + > =C2=A0 =C2=A0 atexit(qemu_run_exit_notifiers); > =C2=A0 =C2=A0 error_set_progname(argv[0]); > > -- > 1.7.9.5 > >