[PATCH] libxc hardening

[PATCH] libxc hardening

[Anthony Liguori]

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

Howdy,

The following patches hardens a good portion of libxc's error path code 
against errno clobbering.  Errno clobbering occurs when a function 
returns a failure code (such as -1) but then calls some other function 
(like munmap or perror) that could potentially change the value of errno.

The patch doesn't touch any of the build/save/restore code because it 
seems like since these functions do so much work it would be useful to 
create special error returns for them.  Does this sound reasonable?

There's also a ton of read() calls that need to be hardened but that's 
another patch.

Signed-off-by: Anthony Liguori

Regards,

Anthony Liguori



[-- Attachment #2: xc_errno_hardening.diff --]
[-- Type: text/x-patch, Size: 8180 bytes --]

diff -ur xen-unstable-1/tools/libxc/xc_evtchn.c xen-unstable/tools/libxc/xc_evtchn.c
--- xen-unstable-1/tools/libxc/xc_evtchn.c	2005-06-22 17:50:44.000000000 -0500
+++ xen-unstable/tools/libxc/xc_evtchn.c	2005-06-22 22:59:39.297821600 -0500
@@ -13,21 +13,29 @@
 {
     int ret = -1;
     privcmd_hypercall_t hypercall;
+    int saved_errno = 0;
 
     hypercall.op     = __HYPERVISOR_event_channel_op;
     hypercall.arg[0] = (unsigned long)op;
 
     if ( mlock(op, sizeof(*op)) != 0 )
     {
+        saved_errno = errno;
         PERROR("do_evtchn_op: op mlock failed");
         goto out;
     }
 
-    if ((ret = do_xen_hypercall(xc_handle, &hypercall)) < 0)
+    if ((ret = do_xen_hypercall(xc_handle, &hypercall)) < 0) {
+        saved_errno = errno;
         ERROR("do_evtchn_op: HYPERVISOR_event_channel_op failed: %d", ret);
+    }
 
     (void)munlock(op, sizeof(*op));
  out:
+
+    if (ret < 0) {
+        errno = saved_errno;
+    }
     return ret;
 }
 
diff -ur xen-unstable-1/tools/libxc/xc_gnttab.c xen-unstable/tools/libxc/xc_gnttab.c
--- xen-unstable-1/tools/libxc/xc_gnttab.c	2005-06-22 17:50:44.000000000 -0500
+++ xen-unstable/tools/libxc/xc_gnttab.c	2005-06-22 23:03:13.451265328 -0500
@@ -18,6 +18,7 @@
 {
     int ret = -1;
     privcmd_hypercall_t hypercall;
+    int saved_errno = 0;
 
     hypercall.op     = __HYPERVISOR_grant_table_op;
     hypercall.arg[0] = cmd;
@@ -26,15 +27,21 @@
 
     if ( mlock(op, sizeof(*op)) != 0 )
     {
+        saved_errno = errno;
         PERROR("do_gnttab_op: op mlock failed");
         goto out;
     }
 
-    if ( (ret = do_xen_hypercall(xc_handle, &hypercall)) < 0 )
+    if ( (ret = do_xen_hypercall(xc_handle, &hypercall)) < 0 ) {
+        saved_errno = errno;
         ERROR("do_gnttab_op: HYPERVISOR_grant_table_op failed: %d", ret);
+    }
 
     (void)munlock(op, sizeof(*op));
  out:
+    if (ret < 0) {
+        errno = saved_errno;
+    }
     return ret;
 }
 
@@ -129,8 +136,11 @@
 int xc_grant_interface_open(void)
 {
     int fd = open("/proc/xen/grant", O_RDWR);
-    if ( fd == -1 )
+    if ( fd == -1 ) {
+        int saved_errno = errno;
         PERROR("Could not obtain handle on grant command interface");
+        errno = saved_errno;
+    }
     return fd;
 
 }
diff -ur xen-unstable-1/tools/libxc/xc_misc.c xen-unstable/tools/libxc/xc_misc.c
--- xen-unstable-1/tools/libxc/xc_misc.c	2005-06-22 17:50:44.000000000 -0500
+++ xen-unstable/tools/libxc/xc_misc.c	2005-06-22 23:05:47.644824344 -0500
@@ -9,8 +9,11 @@
 int xc_interface_open(void)
 {
     int fd = open("/proc/xen/privcmd", O_RDWR);
-    if ( fd == -1 )
+    if ( fd == -1 ) {
+        int saved_errno = errno;
         PERROR("Could not obtain handle on privileged command interface");
+	errno = saved_errno;
+    }
     return fd;
 }
 
@@ -28,6 +31,7 @@
     dom0_op_t op;
     char *buffer = *pbuffer;
     unsigned int nr_chars = *pnr_chars;
+    int saved_errno = 0;
 
     op.cmd = DOM0_READCONSOLE;
     op.u.readconsole.buffer = buffer;
@@ -41,10 +45,14 @@
     {
         *pbuffer   = op.u.readconsole.buffer;
         *pnr_chars = op.u.readconsole.count;
+        saved_errno = errno;
     }
 
     (void)munlock(buffer, nr_chars);
 
+    if (ret != 0) {
+        errno = saved_errno;
+    }
     return ret;
 }    
 
diff -ur xen-unstable-1/tools/libxc/xc_private.c xen-unstable/tools/libxc/xc_private.c
--- xen-unstable-1/tools/libxc/xc_private.c	2005-06-22 17:50:44.000000000 -0500
+++ xen-unstable/tools/libxc/xc_private.c	2005-06-22 23:07:56.069300856 -0500
@@ -16,14 +16,25 @@
     if ( addr == MAP_FAILED )
 	return NULL;
 
+    /* even though NULL is a valid return for mmap(), it's not likely on the
+       platforms we support.  we use null as an invalid return those so just
+       make sure we don't ever get ourselves confused. */
+    if ( addr == NULL ) {
+        munmap(addr, num*PAGE_SIZE);
+        errno = EINVAL;
+        return NULL;
+    }
+
     ioctlx.num=num;
     ioctlx.dom=dom;
     ioctlx.addr=(unsigned long)addr;
     ioctlx.arr=arr;
     if ( ioctl( xc_handle, IOCTL_PRIVCMD_MMAPBATCH, &ioctlx ) < 0 )
     {
-	perror("XXXXXXXX");
+	int saved_errno = errno;
+	PERROR("mmap_batch ioctl failed!");
 	munmap(addr, num*PAGE_SIZE);
+	errno = saved_errno;
 	return NULL;
     }
     return addr;
@@ -43,6 +54,12 @@
     if ( addr == MAP_FAILED )
 	return NULL;
 
+    if ( addr == NULL ) {
+        munmap(addr, size);
+        errno = EINVAL;
+        return NULL;
+    }
+
     ioctlx.num=1;
     ioctlx.dom=dom;
     ioctlx.entry=&entry;
@@ -51,7 +68,9 @@
     entry.npages=(size+PAGE_SIZE-1)>>PAGE_SHIFT;
     if ( ioctl( xc_handle, IOCTL_PRIVCMD_MMAP, &ioctlx ) < 0 )
     {
+	int saved_errno = errno;
 	munmap(addr, size);
+	errno = saved_errno;
 	return NULL;
     }
     return addr;
@@ -82,7 +101,9 @@
     op.u.getpageframeinfo.domain = (domid_t)dom;
     if ( do_dom0_op(xc_handle, &op) < 0 )
     {
+        int saved_errno = errno;
         PERROR("Unexpected failure when getting page frame info!");
+        errno = saved_errno;
         return GETPFN_ERR;
     }
     return op.u.getpageframeinfo.type;
@@ -110,6 +131,7 @@
 {
     int err = 0;
     privcmd_hypercall_t hypercall;
+    int saved_errno = 0;
 
     if ( mmu->idx == 0 )
         return 0;
@@ -122,6 +144,7 @@
 
     if ( mlock(mmu->updates, sizeof(mmu->updates)) != 0 )
     {
+        saved_errno = errno;
         PERROR("flush_mmu_updates: mmu updates mlock failed");
         err = 1;
         goto out;
@@ -129,6 +152,7 @@
 
     if ( do_xen_hypercall(xc_handle, &hypercall) < 0 )
     {
+        saved_errno = errno;
         ERROR("Failure when submitting mmu updates");
         err = 1;
     }
@@ -138,6 +162,10 @@
     (void)munlock(mmu->updates, sizeof(mmu->updates));
 
  out:
+    if (err != 0) {
+        errno = saved_errno;
+    }
+
     return err;
 }
 
@@ -179,7 +207,9 @@
     op.u.getvcpucontext.ctxt   = NULL;
     if ( (do_dom0_op(xc_handle, &op) < 0) )
     {
+        int saved_errno = errno;
         PERROR("Could not get info on domain");
+        errno = saved_errno;
         return -1;
     }
     return op.u.getvcpucontext.cpu_time;
@@ -205,7 +235,9 @@
 
     if ( ioctl( xc_handle, IOCTL_PRIVCMD_GET_MACH2PHYS_START_MFN, &mfn ) < 0 )
     {
-	perror("xc_get_m2p_start_mfn:");
+	int saved_errno = errno;
+	PERROR("xc_get_m2p_start_mfn:");
+	errno = saved_errno;
 	return 0;
     }
     return mfn;
@@ -218,6 +250,8 @@
 {
     dom0_op_t op;
     int ret;
+    int saved_errno;
+
     op.cmd = DOM0_GETMEMLIST;
     op.u.getmemlist.domain   = (domid_t)domid;
     op.u.getmemlist.max_pfns = max_pfns;
@@ -231,6 +265,7 @@
     }    
 
     ret = do_dom0_op(xc_handle, &op);
+    saved_errno = errno;
 
     (void)munlock(pfn_buf, max_pfns * sizeof(unsigned long));
 
@@ -249,6 +284,8 @@
 #endif
 #endif
 
+    if (ret < 0) errno = saved_errno;
+
     return (ret < 0) ? -1 : op.u.getmemlist.num_pfns;
 }
 
@@ -303,33 +340,39 @@
     gzFile kernel_gfd = NULL;
     char *image = NULL;
     unsigned int bytes;
+    int saved_errno = 0;
 
     if ( (kernel_fd = open(filename, O_RDONLY)) < 0 )
     {
+        saved_errno = errno;
         PERROR("Could not open kernel image");
         goto out;
     }
 
     if ( (*size = xc_get_filesz(kernel_fd)) == 0 )
     {
+        saved_errno = errno;
         PERROR("Could not read kernel image");
         goto out;
     }
 
     if ( (kernel_gfd = gzdopen(kernel_fd, "rb")) == NULL )
     {
+        saved_errno = errno;
         PERROR("Could not allocate decompression state for state file");
         goto out;
     }
 
     if ( (image = malloc(*size)) == NULL )
     {
+        saved_errno = errno;
         PERROR("Could not allocate memory for kernel image");
         goto out;
     }
 
     if ( (bytes = gzread(kernel_gfd, image, *size)) != *size )
     {
+        saved_errno = errno;
         PERROR("Error reading kernel image, could not"
                " read the whole image (%d != %ld).", bytes, *size);
         free(image);
@@ -341,6 +384,10 @@
         gzclose(kernel_gfd);
     else if ( kernel_fd >= 0 )
         close(kernel_fd);
+    if (image == NULL) {
+	errno = saved_errno;
+    }
+
     return image;
 }
 

[-- 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] libxc hardening

[Keir Fraser]

On 23 Jun 2005, at 05:15, Anthony Liguori wrote:

> The following patches hardens a good portion of libxc's error path 
> code against errno clobbering.  Errno clobbering occurs when a 
> function returns a failure code (such as -1) but then calls some other 
> function (like munmap or perror) that could potentially change the 
> value of errno.

mmap() will never return NULL address (according to the man page). I 
guess you might be able to force it with MAP_FIXED, but no sane mmap 
implementation would ever automatically choose NULL as a reasonable 
valid return address. :-)

  -- Keir

Re: [PATCH] libxc hardening

[Chris Wright]

* Keir Fraser (Keir.Fraser@cl.cam.ac.uk) wrote:
> mmap() will never return NULL address (according to the man page). I 
> guess you might be able to force it with MAP_FIXED, but no sane mmap 
> implementation would ever automatically choose NULL as a reasonable 
> valid return address. :-)

Heh, sadly we just recently fixed a bug in the kernel that actually
allowed returning NULL w/out MAP_FIXED.  But aside from bugs... ;-)