Skip to contentSkip to conversation

libxc hardening

PATCH In review Anthony Liguori 3 participants 3 messages Jun 23, 2005

lore
Share & tools
lore

Anthony Liguori

author
#0 lore raw

PATCH 1/1 libxc hardening

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

tools/libxc/xc_evtchn.c +9 -1
diff -ur xen-unstable-1/tools/libxc/xc_evtchn.c xen-unstable/tools/libxc/xc_evtchn.c
@@ -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;
 }
tools/libxc/xc_gnttab.c +12 -2
diff -ur xen-unstable-1/tools/libxc/xc_gnttab.c xen-unstable/tools/libxc/xc_gnttab.c
@@ -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;
 
 }
tools/libxc/xc_misc.c +9 -1
diff -ur xen-unstable-1/tools/libxc/xc_misc.c xen-unstable/tools/libxc/xc_misc.c
@@ -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;
 }
tools/libxc/xc_private.c +49 -2

178 lines not on this page · raw message

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#1 lore raw
Quoted 1 line · Anthony Liguori

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

#2 lore raw
Quoted 4 lines · * Keir Fraser ([email protected]) 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... ;-)