Re: [PATCH] virtfs-proxy-helper: switch from libcap to libcap-ng
On 03/12/19 11:34, Greg Kurz wrote: > On Fri, 29 Nov 2019 13:59:37 +0100 > Greg Kurz wrote: > >> On Fri, 29 Nov 2019 13:49:20 +0100 >> Paolo Bonzini wrote: >> >>> On 29/11/19 13:32, Greg Kurz wrote: Nice. :) Reviewed-by: Greg Kurz Paolo, I can take this through my 9p tree if you want. Otherwise, Acked-by: Greg Kurz >>> >>> Yes, please do it since it's self-contained. You'd probably also test >>> it better than me. :) >>> >>> Paolo >>> >> >> Ok I'll just do that then. >> > > And it happens to be missing an extra-change in Makefile :) > > -fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap > +fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap-ng The new line is not needed, -lcap-ng should already be in LIBS via LIBS+=-lz $(LIBS_TOOLS) However, removing -lcap is certainly a good idea. :) Paolo
Re: [PATCH] virtfs-proxy-helper: switch from libcap to libcap-ng
On Fri, 29 Nov 2019 13:59:37 +0100 Greg Kurz wrote: > On Fri, 29 Nov 2019 13:49:20 +0100 > Paolo Bonzini wrote: > > > On 29/11/19 13:32, Greg Kurz wrote: > > > Nice. :) > > > > > > Reviewed-by: Greg Kurz > > > > > > Paolo, > > > > > > I can take this through my 9p tree if you want. Otherwise, > > > > > > Acked-by: Greg Kurz > > > > Yes, please do it since it's self-contained. You'd probably also test > > it better than me. :) > > > > Paolo > > > > Ok I'll just do that then. > And it happens to be missing an extra-change in Makefile :) -fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap +fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap-ng I've fixed this up in my tree. > Cheers, > > -- > Greg
Re: [PATCH] virtfs-proxy-helper: switch from libcap to libcap-ng
On Fri, Nov 29, 2019 at 12:16:32PM +0100, Paolo Bonzini wrote:
> virtfs-proxy-helper is the only user of libcap; everyone else is using
> the simpler libcap-ng API. Switch and remove the configure code to
> detect libcap.
>
> Signed-off-by: Paolo Bonzini
> ---
> configure | 18 +--
> fsdev/virtfs-proxy-helper.c | 100
> 2 files changed, 46 insertions(+), 72 deletions(-)
The dockerfiles can also be updated to remove libcap-dev/devel
>
> diff --git a/configure b/configure
> index afe9393f04..2216662bf6 100755
> --- a/configure
> +++ b/configure
> @@ -3863,22 +3863,6 @@ else
>mpathpersist=no
> fi
>
> -##
> -# libcap probe
> -
> -if test "$cap" != "no" ; then
> - cat > $TMPC < -#include
> -#include
> -int main(void) { cap_t caps; caps = cap_init(); return caps != NULL; }
> -EOF
> - if compile_prog "" "-lcap" ; then
> -cap=yes
> - else
> -cap=no
> - fi
> -fi
> -
> ##
> # pthread probe
> PTHREADLIBS_LIST="-pthread -lpthread -lpthreadGC2"
> @@ -6204,7 +6188,7 @@ if test "$want_tools" = "yes" ; then
> fi
> if test "$softmmu" = yes ; then
>if test "$linux" = yes; then
> -if test "$virtfs" != no && test "$cap" = yes && test "$attr" = yes ; then
> +if test "$virtfs" != no && test "$cap_ng" = yes && test "$attr" = yes ;
> then
>virtfs=yes
>tools="$tools fsdev/virtfs-proxy-helper\$(EXESUF)"
> else
> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> index 6f132c5ff1..0d4de49dcf 100644
> --- a/fsdev/virtfs-proxy-helper.c
> +++ b/fsdev/virtfs-proxy-helper.c
> @@ -13,7 +13,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -21,6 +20,7 @@
> #ifdef CONFIG_LINUX_MAGIC_H
> #include
> #endif
> +#include
> #include "qemu-common.h"
> #include "qemu/sockets.h"
> #include "qemu/xattr.h"
> @@ -79,49 +79,10 @@ static void do_perror(const char *string)
> }
> }
>
> -static int do_cap_set(cap_value_t *cap_value, int size, int reset)
> -{
> -cap_t caps;
> -if (reset) {
> -/*
> - * Start with an empty set and set permitted and effective
> - */
> -caps = cap_init();
> -if (caps == NULL) {
> -do_perror("cap_init");
> -return -1;
> -}
> -if (cap_set_flag(caps, CAP_PERMITTED, size, cap_value, CAP_SET) < 0)
> {
> -do_perror("cap_set_flag");
> -goto error;
> -}
> -} else {
> -caps = cap_get_proc();
> -if (!caps) {
> -do_perror("cap_get_proc");
> -return -1;
> -}
> -}
> -if (cap_set_flag(caps, CAP_EFFECTIVE, size, cap_value, CAP_SET) < 0) {
> -do_perror("cap_set_flag");
> -goto error;
> -}
> -if (cap_set_proc(caps) < 0) {
> -do_perror("cap_set_proc");
> -goto error;
> -}
> -cap_free(caps);
> -return 0;
> -
> -error:
> -cap_free(caps);
> -return -1;
> -}
> -
> static int init_capabilities(void)
> {
> /* helper needs following capabilities only */
> -cap_value_t cap_list[] = {
> +int cap_list[] = {
> CAP_CHOWN,
> CAP_DAC_OVERRIDE,
> CAP_FOWNER,
> @@ -130,7 +91,34 @@ static int init_capabilities(void)
> CAP_MKNOD,
> CAP_SETUID,
> };
> -return do_cap_set(cap_list, ARRAY_SIZE(cap_list), 1);
> +int i;
> +
> +capng_clear(CAPNG_SELECT_BOTH);
> +for (i = 0; i < ARRAY_SIZE(cap_list); i++) {
> +if (capng_update(CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED,
> + cap_list[i]) < 0) {
> +do_perror("capng_update");
> +return -1;
> +}
> +}
> +if (capng_apply(CAPNG_SELECT_BOTH) < 0) {
> +do_perror("capng_apply");
> +return -1;
> +}
> +
> +/* Prepare effective set for setugid. */
> +for (i = 0; i < ARRAY_SIZE(cap_list); i++) {
> +if (cap_list[i] == CAP_DAC_OVERRIDE) {
> +continue;
> +}
> +
> +if (capng_update(CAPNG_DROP, CAPNG_EFFECTIVE,
> + cap_list[i]) < 0) {
> +do_perror("capng_update");
> +return -1;
> +}
> +}
> +return 0;
> }
>
> static int socket_read(int sockfd, void *buff, ssize_t size)
> @@ -295,14 +283,6 @@ static int setugid(int uid, int gid, int *suid, int
> *sgid)
> {
> int retval;
>
> -/*
> - * We still need DAC_OVERRIDE because we don't change
> - * supplementary group ids, and hence may be subjected DAC rules
> - */
> -cap_value_t cap_list[] = {
> -CAP_DAC_OVERRIDE,
> -};
> -
> *suid = geteuid();
> *sgid = getegid();
>
> @@ -316,11 +296,21 @@ static int setugid(int uid, int gid, int *suid, int
> *sgid)
> goto err_sgid;
> }
>
> -if (uid != 0 || gid != 0) {
> -if
Re: [PATCH] virtfs-proxy-helper: switch from libcap to libcap-ng
On Fri, 29 Nov 2019 13:49:20 +0100 Paolo Bonzini wrote: > On 29/11/19 13:32, Greg Kurz wrote: > > Nice. :) > > > > Reviewed-by: Greg Kurz > > > > Paolo, > > > > I can take this through my 9p tree if you want. Otherwise, > > > > Acked-by: Greg Kurz > > Yes, please do it since it's self-contained. You'd probably also test > it better than me. :) > > Paolo > Ok I'll just do that then. Cheers, -- Greg
Re: [PATCH] virtfs-proxy-helper: switch from libcap to libcap-ng
On Fri, 29 Nov 2019 12:16:32 +0100
Paolo Bonzini wrote:
> virtfs-proxy-helper is the only user of libcap; everyone else is using
> the simpler libcap-ng API. Switch and remove the configure code to
> detect libcap.
>
> Signed-off-by: Paolo Bonzini
> ---
Nice. :)
Reviewed-by: Greg Kurz
Paolo,
I can take this through my 9p tree if you want. Otherwise,
Acked-by: Greg Kurz
Also, this calls for some extra cleanup in travis.yml and gitlab-ci.yml
which were recently amended by Thomas to install libcap-dev.
commit c269447f78b7cfb0e85d14bc7bb8cb0d25d19781
Author: Thomas Huth
Date: Thu Sep 5 13:33:46 2019 +0200
travis.yml: Install libcap-dev for testing virito-9p
and
commit e7dc804ef0d7cac9ac8b4a1324ab7dbfafb55704
Author: Thomas Huth
Date: Thu Sep 5 12:36:50 2019 +0200
gitlab-ci.yml: Install libattr-devel and libcap-devel to test virtio-9p
> configure | 18 +--
> fsdev/virtfs-proxy-helper.c | 100
> 2 files changed, 46 insertions(+), 72 deletions(-)
>
> diff --git a/configure b/configure
> index afe9393f04..2216662bf6 100755
> --- a/configure
> +++ b/configure
> @@ -3863,22 +3863,6 @@ else
>mpathpersist=no
> fi
>
> -##
> -# libcap probe
> -
> -if test "$cap" != "no" ; then
> - cat > $TMPC < -#include
> -#include
> -int main(void) { cap_t caps; caps = cap_init(); return caps != NULL; }
> -EOF
> - if compile_prog "" "-lcap" ; then
> -cap=yes
> - else
> -cap=no
> - fi
> -fi
> -
> ##
> # pthread probe
> PTHREADLIBS_LIST="-pthread -lpthread -lpthreadGC2"
> @@ -6204,7 +6188,7 @@ if test "$want_tools" = "yes" ; then
> fi
> if test "$softmmu" = yes ; then
>if test "$linux" = yes; then
> -if test "$virtfs" != no && test "$cap" = yes && test "$attr" = yes ; then
> +if test "$virtfs" != no && test "$cap_ng" = yes && test "$attr" = yes ;
> then
>virtfs=yes
>tools="$tools fsdev/virtfs-proxy-helper\$(EXESUF)"
> else
> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> index 6f132c5ff1..0d4de49dcf 100644
> --- a/fsdev/virtfs-proxy-helper.c
> +++ b/fsdev/virtfs-proxy-helper.c
> @@ -13,7 +13,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -21,6 +20,7 @@
> #ifdef CONFIG_LINUX_MAGIC_H
> #include
> #endif
> +#include
> #include "qemu-common.h"
> #include "qemu/sockets.h"
> #include "qemu/xattr.h"
> @@ -79,49 +79,10 @@ static void do_perror(const char *string)
> }
> }
>
> -static int do_cap_set(cap_value_t *cap_value, int size, int reset)
> -{
> -cap_t caps;
> -if (reset) {
> -/*
> - * Start with an empty set and set permitted and effective
> - */
> -caps = cap_init();
> -if (caps == NULL) {
> -do_perror("cap_init");
> -return -1;
> -}
> -if (cap_set_flag(caps, CAP_PERMITTED, size, cap_value, CAP_SET) < 0)
> {
> -do_perror("cap_set_flag");
> -goto error;
> -}
> -} else {
> -caps = cap_get_proc();
> -if (!caps) {
> -do_perror("cap_get_proc");
> -return -1;
> -}
> -}
> -if (cap_set_flag(caps, CAP_EFFECTIVE, size, cap_value, CAP_SET) < 0) {
> -do_perror("cap_set_flag");
> -goto error;
> -}
> -if (cap_set_proc(caps) < 0) {
> -do_perror("cap_set_proc");
> -goto error;
> -}
> -cap_free(caps);
> -return 0;
> -
> -error:
> -cap_free(caps);
> -return -1;
> -}
> -
> static int init_capabilities(void)
> {
> /* helper needs following capabilities only */
> -cap_value_t cap_list[] = {
> +int cap_list[] = {
> CAP_CHOWN,
> CAP_DAC_OVERRIDE,
> CAP_FOWNER,
> @@ -130,7 +91,34 @@ static int init_capabilities(void)
> CAP_MKNOD,
> CAP_SETUID,
> };
> -return do_cap_set(cap_list, ARRAY_SIZE(cap_list), 1);
> +int i;
> +
> +capng_clear(CAPNG_SELECT_BOTH);
> +for (i = 0; i < ARRAY_SIZE(cap_list); i++) {
> +if (capng_update(CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED,
> + cap_list[i]) < 0) {
> +do_perror("capng_update");
> +return -1;
> +}
> +}
> +if (capng_apply(CAPNG_SELECT_BOTH) < 0) {
> +do_perror("capng_apply");
> +return -1;
> +}
> +
> +/* Prepare effective set for setugid. */
> +for (i = 0; i < ARRAY_SIZE(cap_list); i++) {
> +if (cap_list[i] == CAP_DAC_OVERRIDE) {
> +continue;
> +}
> +
> +if (capng_update(CAPNG_DROP, CAPNG_EFFECTIVE,
> + cap_list[i]) < 0) {
> +do_perror("capng_update");
> +return -1;
> +}
> +}
> +return 0;
> }
>
> static int socket_read(int sockfd, void *buff, ssize_t size)
> @@ -295,14 +283,6 @@
Re: [PATCH] virtfs-proxy-helper: switch from libcap to libcap-ng
On 29/11/19 13:32, Greg Kurz wrote: > Nice. :) > > Reviewed-by: Greg Kurz > > Paolo, > > I can take this through my 9p tree if you want. Otherwise, > > Acked-by: Greg Kurz Yes, please do it since it's self-contained. You'd probably also test it better than me. :) Paolo
Re: [PATCH] virtfs-proxy-helper: switch from libcap to libcap-ng
On Fri, Nov 29, 2019 at 12:16:32PM +0100, Paolo Bonzini wrote: > virtfs-proxy-helper is the only user of libcap; everyone else is using > the simpler libcap-ng API. Switch and remove the configure code to > detect libcap. > > Signed-off-by: Paolo Bonzini > --- > configure | 18 +-- > fsdev/virtfs-proxy-helper.c | 100 > 2 files changed, 46 insertions(+), 72 deletions(-) Reviewed-by: Daniel P. Berrangé 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 :|
[PATCH] virtfs-proxy-helper: switch from libcap to libcap-ng
virtfs-proxy-helper is the only user of libcap; everyone else is using
the simpler libcap-ng API. Switch and remove the configure code to
detect libcap.
Signed-off-by: Paolo Bonzini
---
configure | 18 +--
fsdev/virtfs-proxy-helper.c | 100
2 files changed, 46 insertions(+), 72 deletions(-)
diff --git a/configure b/configure
index afe9393f04..2216662bf6 100755
--- a/configure
+++ b/configure
@@ -3863,22 +3863,6 @@ else
mpathpersist=no
fi
-##
-# libcap probe
-
-if test "$cap" != "no" ; then
- cat > $TMPC <
-#include
-int main(void) { cap_t caps; caps = cap_init(); return caps != NULL; }
-EOF
- if compile_prog "" "-lcap" ; then
-cap=yes
- else
-cap=no
- fi
-fi
-
##
# pthread probe
PTHREADLIBS_LIST="-pthread -lpthread -lpthreadGC2"
@@ -6204,7 +6188,7 @@ if test "$want_tools" = "yes" ; then
fi
if test "$softmmu" = yes ; then
if test "$linux" = yes; then
-if test "$virtfs" != no && test "$cap" = yes && test "$attr" = yes ; then
+if test "$virtfs" != no && test "$cap_ng" = yes && test "$attr" = yes ;
then
virtfs=yes
tools="$tools fsdev/virtfs-proxy-helper\$(EXESUF)"
else
diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 6f132c5ff1..0d4de49dcf 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -13,7 +13,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -21,6 +20,7 @@
#ifdef CONFIG_LINUX_MAGIC_H
#include
#endif
+#include
#include "qemu-common.h"
#include "qemu/sockets.h"
#include "qemu/xattr.h"
@@ -79,49 +79,10 @@ static void do_perror(const char *string)
}
}
-static int do_cap_set(cap_value_t *cap_value, int size, int reset)
-{
-cap_t caps;
-if (reset) {
-/*
- * Start with an empty set and set permitted and effective
- */
-caps = cap_init();
-if (caps == NULL) {
-do_perror("cap_init");
-return -1;
-}
-if (cap_set_flag(caps, CAP_PERMITTED, size, cap_value, CAP_SET) < 0) {
-do_perror("cap_set_flag");
-goto error;
-}
-} else {
-caps = cap_get_proc();
-if (!caps) {
-do_perror("cap_get_proc");
-return -1;
-}
-}
-if (cap_set_flag(caps, CAP_EFFECTIVE, size, cap_value, CAP_SET) < 0) {
-do_perror("cap_set_flag");
-goto error;
-}
-if (cap_set_proc(caps) < 0) {
-do_perror("cap_set_proc");
-goto error;
-}
-cap_free(caps);
-return 0;
-
-error:
-cap_free(caps);
-return -1;
-}
-
static int init_capabilities(void)
{
/* helper needs following capabilities only */
-cap_value_t cap_list[] = {
+int cap_list[] = {
CAP_CHOWN,
CAP_DAC_OVERRIDE,
CAP_FOWNER,
@@ -130,7 +91,34 @@ static int init_capabilities(void)
CAP_MKNOD,
CAP_SETUID,
};
-return do_cap_set(cap_list, ARRAY_SIZE(cap_list), 1);
+int i;
+
+capng_clear(CAPNG_SELECT_BOTH);
+for (i = 0; i < ARRAY_SIZE(cap_list); i++) {
+if (capng_update(CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED,
+ cap_list[i]) < 0) {
+do_perror("capng_update");
+return -1;
+}
+}
+if (capng_apply(CAPNG_SELECT_BOTH) < 0) {
+do_perror("capng_apply");
+return -1;
+}
+
+/* Prepare effective set for setugid. */
+for (i = 0; i < ARRAY_SIZE(cap_list); i++) {
+if (cap_list[i] == CAP_DAC_OVERRIDE) {
+continue;
+}
+
+if (capng_update(CAPNG_DROP, CAPNG_EFFECTIVE,
+ cap_list[i]) < 0) {
+do_perror("capng_update");
+return -1;
+}
+}
+return 0;
}
static int socket_read(int sockfd, void *buff, ssize_t size)
@@ -295,14 +283,6 @@ static int setugid(int uid, int gid, int *suid, int *sgid)
{
int retval;
-/*
- * We still need DAC_OVERRIDE because we don't change
- * supplementary group ids, and hence may be subjected DAC rules
- */
-cap_value_t cap_list[] = {
-CAP_DAC_OVERRIDE,
-};
-
*suid = geteuid();
*sgid = getegid();
@@ -316,11 +296,21 @@ static int setugid(int uid, int gid, int *suid, int *sgid)
goto err_sgid;
}
-if (uid != 0 || gid != 0) {
-if (do_cap_set(cap_list, ARRAY_SIZE(cap_list), 0) < 0) {
-retval = -errno;
-goto err_suid;
-}
+if (uid == 0 && gid == 0) {
+/* Linux has already copied the permitted set to the effective set. */
+return 0;
+}
+
+/*
+ * All capabilities have been cleared from the effective set. However
+ * we still need DAC_OVERRIDE because we don't change supplementary
+ * group ids, and hence may be subject to DAC rules.
