PATCH: Set close-on-exec flag for QEMU disks

PATCH: Set close-on-exec flag for QEMU disks

[Daniel P. Berrange]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

QEMU does not currently set the close-on-exec flag after opening its virtual
disk images. This causes problems when it later runs the /etc/xen/qemu-ifup
script because the file descriptors get propagated to networking commands
like brctl / ifconfig. The SELinux policy quite rightly does not allow the
networking scripts to access the virtual disk images, so these inherited
file descriptors for AVC denials to be logged.

The attached patch modifies all the QEMU disk driver backends to make sure
the close-on-exec flag is turned on

  Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

[-- Attachment #2: xen-qemu-closexec.patch --]
[-- Type: text/plain, Size: 7610 bytes --]

diff -r 3ac19fda0bc2 tools/ioemu/block-bochs.c
--- a/tools/ioemu/block-bochs.c	Fri Mar 02 12:11:52 2007 +0000
+++ b/tools/ioemu/block-bochs.c	Fri Mar 02 15:56:36 2007 -0500
@@ -88,7 +88,7 @@ static int bochs_open(BlockDriverState *
 static int bochs_open(BlockDriverState *bs, const char *filename)
 {
     BDRVBochsState *s = bs->opaque;
-    int fd, i;
+    int fd, i, flags;
     struct bochs_header bochs;
 
     fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE);
@@ -97,7 +97,16 @@ static int bochs_open(BlockDriverState *
         if (fd < 0)
             return -1;
     }
-    
+    if ((flags = fcntl(fd, F_GETFD)) < 0) {
+        close(fd);
+	return -1;
+    }
+    flags |= FD_CLOEXEC;
+    if ((fcntl(fd, F_SETFD, flags)) < 0) {
+    	close(fd);
+	return -1;
+    }
+
     bs->read_only = 1; // no write support yet
     
     s->fd = fd;
diff -r 3ac19fda0bc2 tools/ioemu/block-cloop.c
--- a/tools/ioemu/block-cloop.c	Fri Mar 02 12:11:52 2007 +0000
+++ b/tools/ioemu/block-cloop.c	Fri Mar 02 15:57:29 2007 -0500
@@ -53,11 +53,23 @@ static int cloop_open(BlockDriverState *
 static int cloop_open(BlockDriverState *bs, const char *filename)
 {
     BDRVCloopState *s = bs->opaque;
+    int fd, flags;
     uint32_t offsets_size,max_compressed_block_size=1,i;
 
-    s->fd = open(filename, O_RDONLY | O_BINARY | O_LARGEFILE);
-    if (s->fd < 0)
+    fd = open(filename, O_RDONLY | O_BINARY | O_LARGEFILE);
+    if (fd < 0)
         return -1;
+    if ((flags = fcntl(fd, F_GETFD)) < 0) {
+    	close(fd);
+	return -1;
+    }
+    flags |= FD_CLOEXEC;
+    if ((fcntl(fd, F_SETFD, flags)) < 0) {
+    	close(fd);
+	return -1;
+    }
+
+    s->fd = fd;
     bs->read_only = 1;
 
     /* read header */
diff -r 3ac19fda0bc2 tools/ioemu/block-cow.c
--- a/tools/ioemu/block-cow.c	Fri Mar 02 12:11:52 2007 +0000
+++ b/tools/ioemu/block-cow.c	Fri Mar 02 15:53:28 2007 -0500
@@ -65,7 +65,7 @@ static int cow_open(BlockDriverState *bs
 static int cow_open(BlockDriverState *bs, const char *filename)
 {
     BDRVCowState *s = bs->opaque;
-    int fd;
+    int fd, flags;
     struct cow_header_v2 cow_header;
     int64_t size;
 
@@ -75,6 +75,16 @@ static int cow_open(BlockDriverState *bs
         if (fd < 0)
             return -1;
     }
+    if ((flags = fcntl(fd, F_GETFD)) < 0) {
+    	close(fd);
+	return -1;
+    }
+    flags |= FD_CLOEXEC;
+    if ((fcntl(fd, F_SETFD, flags)) < 0) {
+    	close(fd);
+	return -1;
+    }
+
     s->fd = fd;
     /* see if it is a cow image */
     if (read(fd, &cow_header, sizeof(cow_header)) != sizeof(cow_header)) {
diff -r 3ac19fda0bc2 tools/ioemu/block-dmg.c
--- a/tools/ioemu/block-dmg.c	Fri Mar 02 12:11:52 2007 +0000
+++ b/tools/ioemu/block-dmg.c	Fri Mar 02 15:57:51 2007 -0500
@@ -76,13 +76,25 @@ static int dmg_open(BlockDriverState *bs
 static int dmg_open(BlockDriverState *bs, const char *filename)
 {
     BDRVDMGState *s = bs->opaque;
+    int fd, flags;
     off_t info_begin,info_end,last_in_offset,last_out_offset;
     uint32_t count;
     uint32_t max_compressed_size=1,max_sectors_per_chunk=1,i;
 
-    s->fd = open(filename, O_RDONLY | O_BINARY | O_LARGEFILE);
-    if (s->fd < 0)
+    fd = open(filename, O_RDONLY | O_BINARY | O_LARGEFILE);
+    if (fd < 0)
         return -1;
+    if ((flags = fcntl(fd, F_GETFD)) < 0) {
+    	close(fd);
+	return -1;
+    }
+    flags |= FD_CLOEXEC;
+    if ((fcntl(fd, F_SETFD, flags)) < 0) {
+    	close(fd);
+	return -1;
+    }
+
+    s->fd = fd;
     bs->read_only = 1;
     s->n_chunks = 0;
     s->offsets = s->lengths = s->sectors = s->sectorcounts = 0;
diff -r 3ac19fda0bc2 tools/ioemu/block-qcow.c
--- a/tools/ioemu/block-qcow.c	Fri Mar 02 12:11:52 2007 +0000
+++ b/tools/ioemu/block-qcow.c	Fri Mar 02 15:52:42 2007 -0500
@@ -92,7 +92,7 @@ static int qcow_open(BlockDriverState *b
 static int qcow_open(BlockDriverState *bs, const char *filename)
 {
     BDRVQcowState *s = bs->opaque;
-    int fd, len, i, shift;
+    int fd, len, i, shift, flags;
     QCowHeader header;
     
     fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE);
@@ -101,6 +101,16 @@ static int qcow_open(BlockDriverState *b
         if (fd < 0)
             return -1;
     }
+    if ((flags = fcntl(fd, F_GETFD)) < 0) {
+    	close(fd);
+	return -1;
+    }
+    flags |= FD_CLOEXEC;
+    if ((fcntl(fd, F_SETFD, flags)) < 0) {
+    	close(fd);
+	return -1;
+    }
+
     s->fd = fd;
     if (read(fd, &header, sizeof(header)) != sizeof(header))
         goto fail;
diff -r 3ac19fda0bc2 tools/ioemu/block-vmdk.c
--- a/tools/ioemu/block-vmdk.c	Fri Mar 02 12:11:52 2007 +0000
+++ b/tools/ioemu/block-vmdk.c	Fri Mar 02 15:52:16 2007 -0500
@@ -92,7 +92,7 @@ static int vmdk_open(BlockDriverState *b
 static int vmdk_open(BlockDriverState *bs, const char *filename)
 {
     BDRVVmdkState *s = bs->opaque;
-    int fd, i;
+    int fd, i, flags;
     uint32_t magic;
     int l1_size;
 
@@ -103,6 +103,16 @@ static int vmdk_open(BlockDriverState *b
             return -1;
         bs->read_only = 1;
     }
+    if ((flags = fcntl(fd, F_GETFD)) < 0) {
+    	close(fd);
+	return -1;
+    }
+    flags |= FD_CLOEXEC;
+    if ((fcntl(fd, F_SETFD, flags)) < 0) {
+    	close(fd);
+	return -1;
+    }
+
     if (read(fd, &magic, sizeof(magic)) != sizeof(magic))
         goto fail;
     magic = be32_to_cpu(magic);
diff -r 3ac19fda0bc2 tools/ioemu/block-vpc.c
--- a/tools/ioemu/block-vpc.c	Fri Mar 02 12:11:52 2007 +0000
+++ b/tools/ioemu/block-vpc.c	Fri Mar 02 15:55:10 2007 -0500
@@ -89,7 +89,7 @@ static int vpc_open(BlockDriverState *bs
 static int vpc_open(BlockDriverState *bs, const char *filename)
 {
     BDRVVPCState *s = bs->opaque;
-    int fd, i;
+    int fd, i, flags;
     struct vpc_subheader header;
 
     fd = open(filename, O_RDWR | O_BINARY | O_LARGEFILE);
@@ -99,6 +99,16 @@ static int vpc_open(BlockDriverState *bs
             return -1;
     }
     
+    if ((flags = fcntl(fd, F_GETFD)) < 0) {
+	close(fd);
+	return -1;
+    }
+    flags |= FD_CLOEXEC;
+    if ((fcntl(fd, F_SETFD, flags)) < 0) {
+    	close(fd);
+	return -1;
+    }
+
     bs->read_only = 1; // no write support yet
     
     s->fd = fd;
diff -r 3ac19fda0bc2 tools/ioemu/block.c
--- a/tools/ioemu/block.c	Fri Mar 02 12:11:52 2007 +0000
+++ b/tools/ioemu/block.c	Fri Mar 02 15:56:14 2007 -0500
@@ -180,7 +180,7 @@ void get_tmp_filename(char *filename, in
    simplify the BSD case */
 static BlockDriver *find_image_format(const char *filename)
 {
-    int fd, ret, score, score_max;
+    int fd, ret, score, score_max, flags;
     BlockDriver *drv1, *drv;
     uint8_t *buf;
     size_t bufsize = 1024;
@@ -190,6 +190,16 @@ static BlockDriver *find_image_format(co
         buf = NULL;
         ret = 0;
     } else {
+    	if ((flags = fcntl(fd, F_GETFD)) < 0) {
+	    close(fd);
+	    return -1;
+	}
+	flags |= FD_CLOEXEC;
+	if ((fcntl(fd, F_SETFD, flags)) < 0) {
+	    close(fd);
+	    return -1;
+	}
+
 #ifdef DIOCGSECTORSIZE
         {
             unsigned int sectorsize = 512;
@@ -675,7 +685,7 @@ static int raw_open(BlockDriverState *bs
 static int raw_open(BlockDriverState *bs, const char *filename)
 {
     BDRVRawState *s = bs->opaque;
-    int fd;
+    int fd,  flags;
     int64_t size;
 #ifdef _BSD
     struct stat sb;
@@ -692,6 +702,16 @@ static int raw_open(BlockDriverState *bs
             return -1;
         bs->read_only = 1;
     }
+    if ((flags = fcntl(fd, F_GETFD)) < 0) {
+	close(fd);
+	return -1;
+    }
+    flags |= FD_CLOEXEC;
+    if ((fcntl(fd, F_SETFD, flags)) < 0) {
+    	close(fd);
+	return -1;
+    }
+
 #ifdef _BSD
     if (!fstat(fd, &sb) && (S_IFCHR & sb.st_mode)) {
 #ifdef DIOCGMEDIASIZE

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: PATCH: Set close-on-exec flag for QEMU disks

[Keir Fraser]

On 2/3/07 21:40, "Daniel P. Berrange" <berrange@redhat.com> wrote:

> QEMU does not currently set the close-on-exec flag after opening its virtual
> disk images. This causes problems when it later runs the /etc/xen/qemu-ifup
> script because the file descriptors get propagated to networking commands
> like brctl / ifconfig. The SELinux policy quite rightly does not allow the
> networking scripts to access the virtual disk images, so these inherited
> file descriptors for AVC denials to be logged.
> 
> The attached patch modifies all the QEMU disk driver backends to make sure
> the close-on-exec flag is turned on

It would be nicer to implement an open_cloexec() function in e.g., vl.c to
do the open() and fcntl() in one go and in one place.

There are lots of uses of open() throughout the qemu sources and the patch
only fixes up a subset of them -- is this correct?

 -- Keir

Re: PATCH: Set close-on-exec flag for QEMU disks

[Anthony Liguori]

Keir Fraser wrote:
> On 2/3/07 21:40, "Daniel P. Berrange" <berrange@redhat.com> wrote:
>
>   
>> QEMU does not currently set the close-on-exec flag after opening its virtual
>> disk images. This causes problems when it later runs the /etc/xen/qemu-ifup
>> script because the file descriptors get propagated to networking commands
>> like brctl / ifconfig. The SELinux policy quite rightly does not allow the
>> networking scripts to access the virtual disk images, so these inherited
>> file descriptors for AVC denials to be logged.
>>
>> The attached patch modifies all the QEMU disk driver backends to make sure
>> the close-on-exec flag is turned on
>>     
>
> It would be nicer to implement an open_cloexec() function in e.g., vl.c to
> do the open() and fcntl() in one go and in one place.
>   

There are few areas where scripts are executed.  Why not just introduce 
an exec() wrapper that closes file descriptors appropriately.

That makes it less likely that this problem will occur in the future.

Regards,

Anthony Liguori

> There are lots of uses of open() throughout the qemu sources and the patch
> only fixes up a subset of them -- is this correct?
>
>  -- Keir
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
>   

Re: PATCH: Set close-on-exec flag for QEMU disks

[Daniel P. Berrange]

[-- Attachment #1: Type: text/plain, Size: 2095 bytes --]

On Mon, Mar 05, 2007 at 03:23:43PM -0600, Anthony Liguori wrote:
> Keir Fraser wrote:
> >On 2/3/07 21:40, "Daniel P. Berrange" <berrange@redhat.com> wrote:
> >
> >  
> >>QEMU does not currently set the close-on-exec flag after opening its 
> >>virtual
> >>disk images. This causes problems when it later runs the 
> >>/etc/xen/qemu-ifup
> >>script because the file descriptors get propagated to networking commands
> >>like brctl / ifconfig. The SELinux policy quite rightly does not allow the
> >>networking scripts to access the virtual disk images, so these inherited
> >>file descriptors for AVC denials to be logged.
> >>
> >>The attached patch modifies all the QEMU disk driver backends to make sure
> >>the close-on-exec flag is turned on
> >>    
> >
> >It would be nicer to implement an open_cloexec() function in e.g., vl.c to
> >do the open() and fcntl() in one go and in one place.
> >  
> 
> There are few areas where scripts are executed.  Why not just introduce 
> an exec() wrapper that closes file descriptors appropriately.

Looking at the QEMU code in Xen there are only two places where exec is
used, and one of those is for the vncviewer spawning which isn't upstream.
One of them uses execlp() while the other uses execv(). So rather than 
doing a refactoring of code which will lead to even more divergance with
upstream, the attached patch just fixes up those 2 locations to close all
file handles upto sysconf(_SC_OPEN_MAX), with exception of STDIN/OUT/ERR
and the TAP deevice handle.

   Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

I've tested against Xen 3.0.4 in Fedora to validate neither the qemu-ifup
or vncviewer processes receive any extraneous file handles. The attached
patch applies cleanly to xen-unstable.hg too

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

[-- Attachment #2: xen-qemu-close-fds-2.patch --]
[-- Type: text/plain, Size: 1416 bytes --]

diff -r 3ac19fda0bc2 tools/ioemu/vl.c
--- a/tools/ioemu/vl.c	Fri Mar 02 12:11:52 2007 +0000
+++ b/tools/ioemu/vl.c	Tue Mar 06 11:21:52 2007 -0500
@@ -3250,6 +3250,14 @@ static int net_tap_init(VLANState *vlan,
         pid = fork();
         if (pid >= 0) {
             if (pid == 0) {
+                int open_max = sysconf (_SC_OPEN_MAX), i;
+                for (i = 0; i < open_max; i++)
+                    if (i != STDIN_FILENO &&
+                        i != STDOUT_FILENO &&
+                        i != STDERR_FILENO &&
+                        i != fd)
+                        close(i);
+
                 parg = args;
                 *parg++ = (char *)setup_script;
                 *parg++ = ifname;
diff -r 3ac19fda0bc2 tools/ioemu/vnc.c
--- a/tools/ioemu/vnc.c	Fri Mar 02 12:11:52 2007 +0000
+++ b/tools/ioemu/vnc.c	Tue Mar 06 11:21:52 2007 -0500
@@ -1445,7 +1445,7 @@ int vnc_display_init(DisplayState *ds, i
 
 int vnc_start_viewer(int port)
 {
-    int pid;
+    int pid, i, open_max;
     char s[16];
 
     sprintf(s, ":%d", port);
@@ -1456,6 +1456,12 @@ int vnc_start_viewer(int port)
 	exit(1);
 
     case 0:	/* child */
+	open_max = sysconf (_SC_OPEN_MAX);
+	for (i = 0; i < open_max; i++)
+	    if (i != STDIN_FILENO &&
+		i != STDOUT_FILENO &&
+		i != STDERR_FILENO)
+		close(i);
 	execlp("vncviewer", "vncviewer", s, NULL);
 	fprintf(stderr, "vncviewer execlp failed\n");
 	exit(1);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel