1. Patchew
  2. QEMU
  3. View series

[PATCH] virtiofsd: Seccomp: Add 'send' for syslog

Dr. David Alan Gilbert (git) posted 1 patch 3 years, 6 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20201102150750.34565-1-dgilbert@redhat.com
Maintainers: Stefan Hajnoczi <stefanha@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>
tools/virtiofsd/passthrough_seccomp.c | 1 +
1 file changed, 1 insertion(+)
[PATCH] virtiofsd: Seccomp: Add 'send' for syslog
Posted by Dr. David Alan Gilbert (git) 3 years, 6 months ago 
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),
 };
 
-- 
2.28.0
Re: [PATCH] virtiofsd: Seccomp: Add 'send' for syslog
Posted by Philippe Mathieu-Daudé 3 years, 6 months ago 
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),
>  };
>  
>
Re: [PATCH] virtiofsd: Seccomp: Add 'send' for syslog
Posted by Dr. David Alan Gilbert 3 years, 6 months ago 
* 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),
> >  };
> >  
> > 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Re: [PATCH] virtiofsd: Seccomp: Add 'send' for syslog
Posted by Daniel P. Berrangé 3 years, 6 months ago 
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
-- 
|: 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 :|
Re: [PATCH] virtiofsd: Seccomp: Add 'send' for syslog
Posted by Stefan Hajnoczi 3 years, 6 months ago 
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
Re: [PATCH] virtiofsd: Seccomp: Add 'send' for syslog
Posted by Daniel P. Berrangé 3 years, 6 months ago 
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
-- 
|: 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 :|
Re: [PATCH] virtiofsd: Seccomp: Add 'send' for syslog
Posted by Dr. David Alan Gilbert 3 years, 6 months ago 
* 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 :|
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.