diff mbox series

virtiofsd: Seccomp: Add 'send' for syslog

Message ID 20201102150750.34565-1-dgilbert@redhat.com (mailing list archive)
State New, archived
Headers show
Series virtiofsd: Seccomp: Add 'send' for syslog | expand

Commit Message

Dr. David Alan Gilbert Nov. 2, 2020, 3:07 p.m. UTC 
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(+)

Comments

Philippe Mathieu-Daudé Nov. 2, 2020, 3:11 p.m. UTC | #1 
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),
>  };
>  
>
Dr. David Alan Gilbert Nov. 2, 2020, 3:13 p.m. UTC | #2 
* 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),
> >  };
> >  
> > 
>
Daniel P. Berrangé Nov. 2, 2020, 3:17 p.m. UTC | #3 
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
Daniel P. Berrangé Nov. 2, 2020, 3:18 p.m. UTC | #4 
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
Stefan Hajnoczi Nov. 2, 2020, 4:43 p.m. UTC | #5 
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
Dr. David Alan Gilbert Nov. 2, 2020, 6:30 p.m. UTC | #6 
* 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 mbox series

Patch

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),
 };