Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 138 ++-------------------------------------
src/util/virqemu.c | 130 ++++++++++++++++++++++++++++++++++++
src/util/virqemu.h | 7 ++
4 files changed, 144 insertions(+), 132 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 01c2e710cd..d737e4d9e4 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2939,6 +2939,7 @@ virQEMUBuildDriveCommandlineFromJSON;
virQEMUBuildNetdevCommandlineFromJSON;
virQEMUBuildObjectCommandlineFromJSON;
virQEMUBuildQemuImgKeySecretOpts;
+virQEMUFileOpenAs;
# util/virrandom.h
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0f98243fe4..a667eb21bf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -148,11 +148,6 @@ static int qemuDomainObjStart(virConnectPtr conn,
static int qemuDomainManagedSaveLoad(virDomainObjPtr vm,
void *opaque);
-static int qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid,
- bool dynamicOwnership,
- const char *path, int oflags,
- bool *needUnlink);
-
static virQEMUDriverPtr qemu_driver;
/* Looks up the domain object from snapshot and unlocks the
@@ -3062,129 +3057,8 @@ qemuOpenFile(virQEMUDriverPtr driver,
(virParseOwnershipIds(seclabel->label, &user, &group) < 0))
return -1;
- return qemuOpenFileAs(user, group, dynamicOwnership,
- path, oflags, needUnlink);
-}
-
-static int
-qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid,
- bool dynamicOwnership,
- const char *path, int oflags,
- bool *needUnlink)
-{
- struct stat sb;
- bool is_reg = true;
- bool need_unlink = false;
- unsigned int vfoflags = 0;
- int fd = -1;
- int path_shared = virFileIsSharedFS(path);
- uid_t uid = geteuid();
- gid_t gid = getegid();
-
- /* path might be a pre-existing block dev, in which case
- * we need to skip the create step, and also avoid unlink
- * in the failure case */
- if (oflags & O_CREAT) {
- need_unlink = true;
-
- /* Don't force chown on network-shared FS
- * as it is likely to fail. */
- if (path_shared <= 0 || dynamicOwnership)
- vfoflags |= VIR_FILE_OPEN_FORCE_OWNER;
-
- if (stat(path, &sb) == 0) {
- /* It already exists, we don't want to delete it on error */
- need_unlink = false;
-
- is_reg = !!S_ISREG(sb.st_mode);
- /* If the path is regular file which exists
- * already and dynamic_ownership is off, we don't
- * want to change its ownership, just open it as-is */
- if (is_reg && !dynamicOwnership) {
- uid = sb.st_uid;
- gid = sb.st_gid;
- }
- }
- }
-
- /* First try creating the file as root */
- if (!is_reg) {
- if ((fd = open(path, oflags & ~O_CREAT)) < 0) {
- fd = -errno;
- goto error;
- }
- } else {
- if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, uid, gid,
- vfoflags | VIR_FILE_OPEN_NOFORK)) < 0) {
- /* If we failed as root, and the error was permission-denied
- (EACCES or EPERM), assume it's on a network-connected share
- where root access is restricted (eg, root-squashed NFS). If the
- qemu user is non-root, just set a flag to
- bypass security driver shenanigans, and retry the operation
- after doing setuid to qemu user */
- if ((fd != -EACCES && fd != -EPERM) || fallback_uid == geteuid())
- goto error;
-
- /* On Linux we can also verify the FS-type of the directory. */
- switch (path_shared) {
- case 1:
- /* it was on a network share, so we'll continue
- * as outlined above
- */
- break;
-
- case -1:
- virReportSystemError(-fd, oflags & O_CREAT
- ? _("Failed to create file "
- "'%s': couldn't determine fs type")
- : _("Failed to open file "
- "'%s': couldn't determine fs type"),
- path);
- goto cleanup;
-
- case 0:
- default:
- /* local file - log the error returned by virFileOpenAs */
- goto error;
- }
-
- /* If we created the file above, then we need to remove it;
- * otherwise, the next attempt to create will fail. If the
- * file had already existed before we got here, then we also
- * don't want to delete it and allow the following to succeed
- * or fail based on existing protections
- */
- if (need_unlink)
- unlink(path);
-
- /* Retry creating the file as qemu user */
-
- /* Since we're passing different modes... */
- vfoflags |= VIR_FILE_OPEN_FORCE_MODE;
-
- if ((fd = virFileOpenAs(path, oflags,
- S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP,
- fallback_uid, fallback_gid,
- vfoflags | VIR_FILE_OPEN_FORK)) < 0) {
- virReportSystemError(-fd, oflags & O_CREAT
- ? _("Error from child process creating '%s'")
- : _("Error from child process opening '%s'"),
- path);
- goto cleanup;
- }
- }
- }
- cleanup:
- if (needUnlink)
- *needUnlink = need_unlink;
- return fd;
-
- error:
- virReportSystemError(-fd, oflags & O_CREAT
- ? _("Failed to create file '%s'")
- : _("Failed to open file '%s'"),
- path);
- goto cleanup;
+ return virQEMUFileOpenAs(user, group, dynamicOwnership,
+ path, oflags, needUnlink);
}
@@ -3247,9 +3121,9 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
}
}
- fd = qemuOpenFileAs(cfg->user, cfg->group, false, path,
- O_WRONLY | O_TRUNC | O_CREAT | directFlag,
- &needUnlink);
+ fd = virQEMUFileOpenAs(cfg->user, cfg->group, false, path,
+ O_WRONLY | O_TRUNC | O_CREAT | directFlag,
+ &needUnlink);
if (fd < 0)
goto cleanup;
@@ -3824,7 +3698,7 @@ doCoreDump(virQEMUDriverPtr driver,
/* Core dumps usually imply last-ditch analysis efforts are
* desired, so we intentionally do not unlink even if a file was
* created. */
- if ((fd = qemuOpenFileAs(cfg->user, cfg->group, false, path,
+ if ((fd = virQEMUFileOpenAs(cfg->user, cfg->group, false, path,
O_CREAT | O_TRUNC | O_WRONLY | directFlag,
NULL)) < 0)
goto cleanup;
diff --git a/src/util/virqemu.c b/src/util/virqemu.c
index 20cb09d878..e1c8673390 100644
--- a/src/util/virqemu.c
+++ b/src/util/virqemu.c
@@ -22,12 +22,18 @@
#include <config.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
+
#include "virbuffer.h"
#include "virerror.h"
#include "virlog.h"
#include "virqemu.h"
#include "virstring.h"
#include "viralloc.h"
+#include "virfile.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -441,3 +447,127 @@ virQEMUBuildQemuImgKeySecretOpts(virBufferPtr buf,
virBufferAddLit(buf, ",");
}
}
+
+
+int
+virQEMUFileOpenAs(uid_t fallback_uid,
+ gid_t fallback_gid,
+ bool dynamicOwnership,
+ const char *path,
+ int oflags,
+ bool *needUnlink)
+{
+ struct stat sb;
+ bool is_reg = true;
+ bool need_unlink = false;
+ unsigned int vfoflags = 0;
+ int fd = -1;
+ int path_shared = virFileIsSharedFS(path);
+ uid_t uid = geteuid();
+ gid_t gid = getegid();
+
+ /* path might be a pre-existing block dev, in which case
+ * we need to skip the create step, and also avoid unlink
+ * in the failure case */
+ if (oflags & O_CREAT) {
+ need_unlink = true;
+
+ /* Don't force chown on network-shared FS
+ * as it is likely to fail. */
+ if (path_shared <= 0 || dynamicOwnership)
+ vfoflags |= VIR_FILE_OPEN_FORCE_OWNER;
+
+ if (stat(path, &sb) == 0) {
+ /* It already exists, we don't want to delete it on error */
+ need_unlink = false;
+
+ is_reg = !!S_ISREG(sb.st_mode);
+ /* If the path is regular file which exists
+ * already and dynamic_ownership is off, we don't
+ * want to change its ownership, just open it as-is */
+ if (is_reg && !dynamicOwnership) {
+ uid = sb.st_uid;
+ gid = sb.st_gid;
+ }
+ }
+ }
+
+ /* First try creating the file as root */
+ if (!is_reg) {
+ if ((fd = open(path, oflags & ~O_CREAT)) < 0) {
+ fd = -errno;
+ goto error;
+ }
+ } else {
+ if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, uid, gid,
+ vfoflags | VIR_FILE_OPEN_NOFORK)) < 0) {
+ /* If we failed as root, and the error was permission-denied
+ (EACCES or EPERM), assume it's on a network-connected share
+ where root access is restricted (eg, root-squashed NFS). If the
+ qemu user is non-root, just set a flag to
+ bypass security driver shenanigans, and retry the operation
+ after doing setuid to qemu user */
+ if ((fd != -EACCES && fd != -EPERM) || fallback_uid == geteuid())
+ goto error;
+
+ /* On Linux we can also verify the FS-type of the directory. */
+ switch (path_shared) {
+ case 1:
+ /* it was on a network share, so we'll continue
+ * as outlined above
+ */
+ break;
+
+ case -1:
+ virReportSystemError(-fd, oflags & O_CREAT
+ ? _("Failed to create file "
+ "'%s': couldn't determine fs type")
+ : _("Failed to open file "
+ "'%s': couldn't determine fs type"),
+ path);
+ goto cleanup;
+
+ case 0:
+ default:
+ /* local file - log the error returned by virFileOpenAs */
+ goto error;
+ }
+
+ /* If we created the file above, then we need to remove it;
+ * otherwise, the next attempt to create will fail. If the
+ * file had already existed before we got here, then we also
+ * don't want to delete it and allow the following to succeed
+ * or fail based on existing protections
+ */
+ if (need_unlink)
+ unlink(path);
+
+ /* Retry creating the file as qemu user */
+
+ /* Since we're passing different modes... */
+ vfoflags |= VIR_FILE_OPEN_FORCE_MODE;
+
+ if ((fd = virFileOpenAs(path, oflags,
+ S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP,
+ fallback_uid, fallback_gid,
+ vfoflags | VIR_FILE_OPEN_FORK)) < 0) {
+ virReportSystemError(-fd, oflags & O_CREAT
+ ? _("Error from child process creating '%s'")
+ : _("Error from child process opening '%s'"),
+ path);
+ goto cleanup;
+ }
+ }
+ }
+ cleanup:
+ if (needUnlink)
+ *needUnlink = need_unlink;
+ return fd;
+
+ error:
+ virReportSystemError(-fd, oflags & O_CREAT
+ ? _("Failed to create file '%s'")
+ : _("Failed to open file '%s'"),
+ path);
+ goto cleanup;
+}
diff --git a/src/util/virqemu.h b/src/util/virqemu.h
index b1296cb657..e4e071f7c5 100644
--- a/src/util/virqemu.h
+++ b/src/util/virqemu.h
@@ -63,3 +63,10 @@ void virQEMUBuildQemuImgKeySecretOpts(virBufferPtr buf,
virStorageEncryptionInfoDefPtr enc,
const char *alias)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+
+int virQEMUFileOpenAs(uid_t fallback_uid,
+ gid_t fallback_gid,
+ bool dynamicOwnership,
+ const char *path,
+ int oflags,
+ bool *needUnlink);
--
2.26.2
On Thu, Aug 06, 2020 at 11:55:12AM +0200, Peter Krempa wrote:
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_driver.c | 138 ++-------------------------------------
> src/util/virqemu.c | 130 ++++++++++++++++++++++++++++++++++++
> src/util/virqemu.h | 7 ++
> 4 files changed, 144 insertions(+), 132 deletions(-)
The original source files were not built on Win32, the new
source files are. This causes a build failure due to...
> diff --git a/src/util/virqemu.c b/src/util/virqemu.c
> index 20cb09d878..e1c8673390 100644
> --- a/src/util/virqemu.c
> +++ b/src/util/virqemu.c
> @@ -22,12 +22,18 @@
>
> #include <config.h>
>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +
> #include "virbuffer.h"
> #include "virerror.h"
> #include "virlog.h"
> #include "virqemu.h"
> #include "virstring.h"
> #include "viralloc.h"
> +#include "virfile.h"
>
> #define VIR_FROM_THIS VIR_FROM_NONE
>
> @@ -441,3 +447,127 @@ virQEMUBuildQemuImgKeySecretOpts(virBufferPtr buf,
> virBufferAddLit(buf, ",");
> }
> }
> +
> +
> +int
> +virQEMUFileOpenAs(uid_t fallback_uid,
> + gid_t fallback_gid,
> + bool dynamicOwnership,
> + const char *path,
> + int oflags,
> + bool *needUnlink)
> +{
> + struct stat sb;
> + bool is_reg = true;
> + bool need_unlink = false;
> + unsigned int vfoflags = 0;
> + int fd = -1;
> + int path_shared = virFileIsSharedFS(path);
> + uid_t uid = geteuid();
> + gid_t gid = getegid();
... these calls
We'll need to provide a stub that raises an error on win32 for this.
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 :|
On Mon, Aug 24, 2020 at 16:18:40 +0100, Daniel Berrange wrote:
> On Thu, Aug 06, 2020 at 11:55:12AM +0200, Peter Krempa wrote:
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> > src/libvirt_private.syms | 1 +
> > src/qemu/qemu_driver.c | 138 ++-------------------------------------
> > src/util/virqemu.c | 130 ++++++++++++++++++++++++++++++++++++
> > src/util/virqemu.h | 7 ++
> > 4 files changed, 144 insertions(+), 132 deletions(-)
>
> The original source files were not built on Win32, the new
> source files are. This causes a build failure due to...
Sigh. I thought that I could make it to be an universal helper. Not very
useful, but universal.
An alternative solution could be to dump it to qemu_domain.c ...
>
> > diff --git a/src/util/virqemu.c b/src/util/virqemu.c
> > index 20cb09d878..e1c8673390 100644
> > --- a/src/util/virqemu.c
> > +++ b/src/util/virqemu.c
> > @@ -22,12 +22,18 @@
> >
> > #include <config.h>
> >
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > +
> > #include "virbuffer.h"
> > #include "virerror.h"
> > #include "virlog.h"
> > #include "virqemu.h"
> > #include "virstring.h"
> > #include "viralloc.h"
> > +#include "virfile.h"
> >
> > #define VIR_FROM_THIS VIR_FROM_NONE
> >
> > @@ -441,3 +447,127 @@ virQEMUBuildQemuImgKeySecretOpts(virBufferPtr buf,
> > virBufferAddLit(buf, ",");
> > }
> > }
> > +
> > +
> > +int
> > +virQEMUFileOpenAs(uid_t fallback_uid,
> > + gid_t fallback_gid,
> > + bool dynamicOwnership,
> > + const char *path,
> > + int oflags,
> > + bool *needUnlink)
> > +{
> > + struct stat sb;
> > + bool is_reg = true;
> > + bool need_unlink = false;
> > + unsigned int vfoflags = 0;
> > + int fd = -1;
> > + int path_shared = virFileIsSharedFS(path);
> > + uid_t uid = geteuid();
> > + gid_t gid = getegid();
>
> ... these calls
>
>
> We'll need to provide a stub that raises an error on win32 for this.
... to avoid a need for this. It's not as universal of a helper anyways.
On Mon, Aug 24, 2020 at 05:24:51PM +0200, Peter Krempa wrote: > On Mon, Aug 24, 2020 at 16:18:40 +0100, Daniel Berrange wrote: > > On Thu, Aug 06, 2020 at 11:55:12AM +0200, Peter Krempa wrote: > > > Signed-off-by: Peter Krempa <pkrempa@redhat.com> > > > --- > > > src/libvirt_private.syms | 1 + > > > src/qemu/qemu_driver.c | 138 ++------------------------------------- > > > src/util/virqemu.c | 130 ++++++++++++++++++++++++++++++++++++ > > > src/util/virqemu.h | 7 ++ > > > 4 files changed, 144 insertions(+), 132 deletions(-) > > > > The original source files were not built on Win32, the new > > source files are. This causes a build failure due to... > > Sigh. I thought that I could make it to be an universal helper. Not very > useful, but universal. > > An alternative solution could be to dump it to qemu_domain.c ... Yeah, I actually thought about this after sending the mail. It isn't used anywhere else outside of QEMU. 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 :|
On a Monday in 2020, Daniel P. Berrangé wrote: >On Mon, Aug 24, 2020 at 05:24:51PM +0200, Peter Krempa wrote: >> On Mon, Aug 24, 2020 at 16:18:40 +0100, Daniel Berrange wrote: >> > On Thu, Aug 06, 2020 at 11:55:12AM +0200, Peter Krempa wrote: >> > > Signed-off-by: Peter Krempa <pkrempa@redhat.com> >> > > --- >> > > src/libvirt_private.syms | 1 + >> > > src/qemu/qemu_driver.c | 138 ++------------------------------------- >> > > src/util/virqemu.c | 130 ++++++++++++++++++++++++++++++++++++ >> > > src/util/virqemu.h | 7 ++ >> > > 4 files changed, 144 insertions(+), 132 deletions(-) >> > >> > The original source files were not built on Win32, the new >> > source files are. This causes a build failure due to... >> >> Sigh. I thought that I could make it to be an universal helper. Not very >> useful, but universal. >> >> An alternative solution could be to dump it to qemu_domain.c ... > How about qemu_util.c? qemu_domain.c already is a dump. Jano >Yeah, I actually thought about this after sending the mail. It isn't >used anywhere else outside of QEMU. > > >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 :| >
On Tue, Aug 25, 2020 at 11:15:11 +0200, Ján Tomko wrote: > On a Monday in 2020, Daniel P. Berrangé wrote: > > On Mon, Aug 24, 2020 at 05:24:51PM +0200, Peter Krempa wrote: > > > On Mon, Aug 24, 2020 at 16:18:40 +0100, Daniel Berrange wrote: > > > > On Thu, Aug 06, 2020 at 11:55:12AM +0200, Peter Krempa wrote: > > > > > Signed-off-by: Peter Krempa <pkrempa@redhat.com> > > > > > --- > > > > > src/libvirt_private.syms | 1 + > > > > > src/qemu/qemu_driver.c | 138 ++------------------------------------- > > > > > src/util/virqemu.c | 130 ++++++++++++++++++++++++++++++++++++ > > > > > src/util/virqemu.h | 7 ++ > > > > > 4 files changed, 144 insertions(+), 132 deletions(-) > > > > > > > > The original source files were not built on Win32, the new > > > > source files are. This causes a build failure due to... > > > > > > Sigh. I thought that I could make it to be an universal helper. Not very > > > useful, but universal. > > > > > > An alternative solution could be to dump it to qemu_domain.c ... > > > > How about qemu_util.c? > > qemu_domain.c already is a dump. Yes, I've already mentioned that in the commit moving it to qemu_domain.c (for now). The thing is that qemu_util doesn't exist at this point, it would also increase confusion of the differenece between qemu_util.c and util/virq emu.c, which I'd like to avoid in a build fix. Additionally it's used mostly by qemuDomainOpenFile which kind of belongs to qemu_domain.c. If we consider that the confusion between qemu_util.c and util/virqemu.c is not big enough to add qemu_util.c, I'm willing to make virQEMUFileOpenAs the first thing to move there.
© 2016 - 2026 Red Hat, Inc.