Message ID | 20201102150750.34565-1-dgilbert@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | virtiofsd: Seccomp: Add 'send' for syslog | expand |
On 11/2/20 4:07 PM, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > On ppc it looks like syslog ends up using 'send' rather than 'sendto'. > > Reference: https://github.com/kata-containers/kata-containers/issues/1050 > > Reported-by: amulmek1@in.ibm.com > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > tools/virtiofsd/passthrough_seccomp.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c > index eb9af8265f..672fb72a31 100644 > --- a/tools/virtiofsd/passthrough_seccomp.c > +++ b/tools/virtiofsd/passthrough_seccomp.c > @@ -118,6 +118,7 @@ static const int syscall_whitelist[] = { > > /* Syscalls used when --syslog is enabled */ > static const int syscall_whitelist_syslog[] = { Is it worth restricting the syscall to POWER? #if defined(__powerpc64__) > + SCMP_SYS(send), #endif > SCMP_SYS(sendto), > }; > >
* Philippe Mathieu-Daudé (philmd@redhat.com) wrote: > On 11/2/20 4:07 PM, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > On ppc it looks like syslog ends up using 'send' rather than 'sendto'. > > > > Reference: https://github.com/kata-containers/kata-containers/issues/1050 > > > > Reported-by: amulmek1@in.ibm.com > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > tools/virtiofsd/passthrough_seccomp.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c > > index eb9af8265f..672fb72a31 100644 > > --- a/tools/virtiofsd/passthrough_seccomp.c > > +++ b/tools/virtiofsd/passthrough_seccomp.c > > @@ -118,6 +118,7 @@ static const int syscall_whitelist[] = { > > > > /* Syscalls used when --syslog is enabled */ > > static const int syscall_whitelist_syslog[] = { > > Is it worth restricting the syscall to POWER? > > #if defined(__powerpc64__) I don't think so, since it's legal for a libc to use either; so any other libc or architecture could choose either to use. Dave > > + SCMP_SYS(send), > > #endif > > > SCMP_SYS(sendto), > > }; > > > > >
On Mon, Nov 02, 2020 at 03:07:50PM +0000, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > On ppc it looks like syslog ends up using 'send' rather than 'sendto'. > > Reference: https://github.com/kata-containers/kata-containers/issues/1050 > > Reported-by: amulmek1@in.ibm.com > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > tools/virtiofsd/passthrough_seccomp.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c > index eb9af8265f..672fb72a31 100644 > --- a/tools/virtiofsd/passthrough_seccomp.c > +++ b/tools/virtiofsd/passthrough_seccomp.c > @@ -118,6 +118,7 @@ static const int syscall_whitelist[] = { > > /* Syscalls used when --syslog is enabled */ > static const int syscall_whitelist_syslog[] = { > + SCMP_SYS(send), > SCMP_SYS(sendto), > }; With glibc, syslog() calls __send() which for Linux target is implemented as: ssize_t __libc_send (int fd, const void *buf, size_t len, int flags) { #ifdef __ASSUME_SEND_SYSCALL return SYSCALL_CANCEL (send, fd, buf, len, flags); #elif defined __ASSUME_SENDTO_SYSCALL return SYSCALL_CANCEL (sendto, fd, buf, len, flags, NULL, 0); #else return SOCKETCALL_CANCEL (send, fd, buf, len, flags); #endif } We can see those defines being referenced vary per architecture: $ git grep -E '__ASSUME_SEND(TO)?_SYSCALL' sysdeps/ sysdeps/unix/sysv/linux/alpha/kernel-features.h:#define __ASSUME_SEND_SYSCALL 1 sysdeps/unix/sysv/linux/arm/kernel-features.h:#define __ASSUME_SEND_SYSCALL 1 sysdeps/unix/sysv/linux/hppa/kernel-features.h:#define __ASSUME_SEND_SYSCALL 1 sysdeps/unix/sysv/linux/i386/kernel-features.h:# undef __ASSUME_SENDTO_SYSCALL sysdeps/unix/sysv/linux/ia64/kernel-features.h:#define __ASSUME_SEND_SYSCALL 1 sysdeps/unix/sysv/linux/kernel-features.h:#define __ASSUME_SENDTO_SYSCALL 1 sysdeps/unix/sysv/linux/m68k/kernel-features.h:# undef __ASSUME_SENDTO_SYSCALL sysdeps/unix/sysv/linux/microblaze/kernel-features.h:#define __ASSUME_SEND_SYSCALL 1 sysdeps/unix/sysv/linux/mips/kernel-features.h:# define __ASSUME_SEND_SYSCALL 1 sysdeps/unix/sysv/linux/powerpc/kernel-features.h:#define __ASSUME_SEND_SYSCALL 1 sysdeps/unix/sysv/linux/s390/kernel-features.h:# undef __ASSUME_SENDTO_SYSCALL sysdeps/unix/sysv/linux/send.c:#ifdef __ASSUME_SEND_SYSCALL sysdeps/unix/sysv/linux/send.c:#elif defined __ASSUME_SENDTO_SYSCALL sysdeps/unix/sysv/linux/sendto.c:#ifdef __ASSUME_SENDTO_SYSCALL sysdeps/unix/sysv/linux/sh/kernel-features.h:#define __ASSUME_SEND_SYSCALL 1 sysdeps/unix/sysv/linux/sparc/kernel-features.h:# undef __ASSUME_SENDTO_SYSCALL So the patch is correct, but the commit message could be updated becase this isn't specific to PPC. Any platform except x86, s490, m68k will use send() rather than sendto() based on what I see here. With any commit message update, you can add Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
On Mon, Nov 02, 2020 at 04:11:44PM +0100, Philippe Mathieu-Daudé wrote: > On 11/2/20 4:07 PM, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > On ppc it looks like syslog ends up using 'send' rather than 'sendto'. > > > > Reference: https://github.com/kata-containers/kata-containers/issues/1050 > > > > Reported-by: amulmek1@in.ibm.com > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > tools/virtiofsd/passthrough_seccomp.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c > > index eb9af8265f..672fb72a31 100644 > > --- a/tools/virtiofsd/passthrough_seccomp.c > > +++ b/tools/virtiofsd/passthrough_seccomp.c > > @@ -118,6 +118,7 @@ static const int syscall_whitelist[] = { > > > > /* Syscalls used when --syslog is enabled */ > > static const int syscall_whitelist_syslog[] = { > > Is it worth restricting the syscall to POWER? That would be wrong, as this is needed on multiple arches. There is no real security benefit to restricting it either, as both syscalls give you broadly equivalent abilities. > > #if defined(__powerpc64__) > > > + SCMP_SYS(send), > > #endif > > > SCMP_SYS(sendto), > > }; Regards, Daniel
On Mon, Nov 02, 2020 at 03:18:24PM +0000, Daniel P. Berrangé wrote: > On Mon, Nov 02, 2020 at 04:11:44PM +0100, Philippe Mathieu-Daudé wrote: > > On 11/2/20 4:07 PM, Dr. David Alan Gilbert (git) wrote: > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > On ppc it looks like syslog ends up using 'send' rather than 'sendto'. > > > > > > Reference: https://github.com/kata-containers/kata-containers/issues/1050 > > > > > > Reported-by: amulmek1@in.ibm.com > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > --- > > > tools/virtiofsd/passthrough_seccomp.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c > > > index eb9af8265f..672fb72a31 100644 > > > --- a/tools/virtiofsd/passthrough_seccomp.c > > > +++ b/tools/virtiofsd/passthrough_seccomp.c > > > @@ -118,6 +118,7 @@ static const int syscall_whitelist[] = { > > > > > > /* Syscalls used when --syslog is enabled */ > > > static const int syscall_whitelist_syslog[] = { > > > > Is it worth restricting the syscall to POWER? > > That would be wrong, as this is needed on multiple arches. > > There is no real security benefit to restricting it either, as both > syscalls give you broadly equivalent abilities. Restricting send to architectures where glibc slightly decreases the host kernel attack surface. I think it makes sense from a security perspective. There could be a send(2)-only bug in Linux isn't present in sendto(2). But there are other issues with seccomp - are we really sure send(2) is never called? Since we don't control the entire binary (share libraries are used, virtiofsd could be linked against another libc, etc) we may not have enough information to conclude that can be eliminated send(2). Stefan
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Mon, Nov 02, 2020 at 03:07:50PM +0000, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > On ppc it looks like syslog ends up using 'send' rather than 'sendto'. > > > > Reference: https://github.com/kata-containers/kata-containers/issues/1050 > > > > Reported-by: amulmek1@in.ibm.com > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > tools/virtiofsd/passthrough_seccomp.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c > > index eb9af8265f..672fb72a31 100644 > > --- a/tools/virtiofsd/passthrough_seccomp.c > > +++ b/tools/virtiofsd/passthrough_seccomp.c > > @@ -118,6 +118,7 @@ static const int syscall_whitelist[] = { > > > > /* Syscalls used when --syslog is enabled */ > > static const int syscall_whitelist_syslog[] = { > > + SCMP_SYS(send), > > SCMP_SYS(sendto), > > }; > > With glibc, syslog() calls __send() which for Linux target is implemented > as: > > > ssize_t > __libc_send (int fd, const void *buf, size_t len, int flags) > { > #ifdef __ASSUME_SEND_SYSCALL > return SYSCALL_CANCEL (send, fd, buf, len, flags); > #elif defined __ASSUME_SENDTO_SYSCALL > return SYSCALL_CANCEL (sendto, fd, buf, len, flags, NULL, 0); > #else > return SOCKETCALL_CANCEL (send, fd, buf, len, flags); > #endif > } > > We can see those defines being referenced vary per architecture: > > $ git grep -E '__ASSUME_SEND(TO)?_SYSCALL' sysdeps/ > sysdeps/unix/sysv/linux/alpha/kernel-features.h:#define __ASSUME_SEND_SYSCALL 1 > sysdeps/unix/sysv/linux/arm/kernel-features.h:#define __ASSUME_SEND_SYSCALL 1 > sysdeps/unix/sysv/linux/hppa/kernel-features.h:#define __ASSUME_SEND_SYSCALL 1 > sysdeps/unix/sysv/linux/i386/kernel-features.h:# undef __ASSUME_SENDTO_SYSCALL > sysdeps/unix/sysv/linux/ia64/kernel-features.h:#define __ASSUME_SEND_SYSCALL 1 > sysdeps/unix/sysv/linux/kernel-features.h:#define __ASSUME_SENDTO_SYSCALL 1 > sysdeps/unix/sysv/linux/m68k/kernel-features.h:# undef __ASSUME_SENDTO_SYSCALL > sysdeps/unix/sysv/linux/microblaze/kernel-features.h:#define __ASSUME_SEND_SYSCALL 1 > sysdeps/unix/sysv/linux/mips/kernel-features.h:# define __ASSUME_SEND_SYSCALL 1 > sysdeps/unix/sysv/linux/powerpc/kernel-features.h:#define __ASSUME_SEND_SYSCALL 1 > sysdeps/unix/sysv/linux/s390/kernel-features.h:# undef __ASSUME_SENDTO_SYSCALL > sysdeps/unix/sysv/linux/send.c:#ifdef __ASSUME_SEND_SYSCALL > sysdeps/unix/sysv/linux/send.c:#elif defined __ASSUME_SENDTO_SYSCALL > sysdeps/unix/sysv/linux/sendto.c:#ifdef __ASSUME_SENDTO_SYSCALL > sysdeps/unix/sysv/linux/sh/kernel-features.h:#define __ASSUME_SEND_SYSCALL 1 > sysdeps/unix/sysv/linux/sparc/kernel-features.h:# undef __ASSUME_SENDTO_SYSCALL > > > So the patch is correct, but the commit message could be updated becase > this isn't specific to PPC. Any platform except x86, s490, m68k will > use send() rather than sendto() based on what I see here. > > With any commit message update, you can add > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Thanks, changed to: On ppc, and some other archs, it looks like syslog ends up using 'send' rather than 'sendto'. Dave > > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| > >
diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c index eb9af8265f..672fb72a31 100644 --- a/tools/virtiofsd/passthrough_seccomp.c +++ b/tools/virtiofsd/passthrough_seccomp.c @@ -118,6 +118,7 @@ static const int syscall_whitelist[] = { /* Syscalls used when --syslog is enabled */ static const int syscall_whitelist_syslog[] = { + SCMP_SYS(send), SCMP_SYS(sendto), };