August 2009 - Devel - libvirt List Archives

[libvirt] error: failed to connect to the hypervisor
by Dominik Klein 21 Aug '09

21 Aug '09

Hi I am running kvm-78 with libvirt 0.6.1 and see the following error from time to time when my linux-ha cluster tries to run "virsh domstate" to figure out a virtual machine's state: error: failed to connect to the hypervisor In syslog, I see: libvirtd: 23:39:11.357: error : Failed to set close-on-exec file descriptor flag Due to this error, the cluster assumes the machine is not running okay and reboots it in order to fix things. This happened twice in 2 months with 10 machines being queried like that each minute (read: not too well reproducable) What is this and how could I work around it or is this fixed in newer versions and so worth an upgrade? Regards Dominik

2 3

[libvirt] Migration fail
by Christian Hennig 21 Aug '09

21 Aug '09

hi, i am trying to migrate a VM from my local system "pc38" to a remote on "pc36": virsh # list --all Id Name State ---------------------------------- - scalaris1 shut off virsh # migrate scalaris1 qemu+ssh://csr-user@pc36/system error: monitor socket did not show up.: No such file or directory What is the monitor socket? Both system are ubuntu server 9.04 : christian@pc38:~$ virsh --version Connecting to uri: qemu:///system 0.6.1 christian@pc38:~$ Thx for help! Christian

2 1

Re: [libvirt] libvirt with libssh2?
by Emre Erenoglu 20 Aug '09

20 Aug '09

> > ---------- Forwarded message ---------- > From: Daniel Veillard <veillard(a)redhat.com> > To: Emre Erenoglu <erenoglu(a)gmail.com> > Date: Thu, 20 Aug 2009 11:14:53 +0200 > Subject: Re: [libvirt] libvirt with libssh2? > On Thu, Aug 20, 2009 at 10:50:37AM +0200, Emre Erenoglu wrote: > > Hi, > > I'm the package maintainer of libvirt for the Pardus distribution. While > > trying to update the package to the latest 0.7.0, I recognized that > > configure script does not find libssh. > > > > I checked our system and found out that we are using libssh2 instead of > > libssh. > > > > >From this website (http://www.libssh2.org/libssh2-vs-libssh.html) > libssh2 > > developers summarize their differences. > > > > My questions: > > 1) Is there a specific reason why we are using libssh? Is there a patch > to > > make libvirt use libssh2 instead? > > 2) What functionality do we loose if we don't have libssh in the system? > > (depending on this, I'll also package libssh or not) > > I guess you activated --with-phyp which currently requires libssh, > but this will be fixed in the future, the result is just that that > driver gets desactivated for now if you don't have libssh > > see http://www.mail-archive.com/libvir-list@redhat.com/msg15086.html > Thanks Daniel. I'm a bit foreign to this "Power Hypervisor" thing, but if I'm not wrong from the information that I got from this site: http://publib.boulder.ibm.com/infocenter/systems/scope/hw/index.jsp?topic=/… than it's nothing that we support in Pardus at the moment. In fact, we only support KVM in our libvirt at the moment, possibly virtualbox in the future. In this case, I'm disabling this pyhp as well as the libssh dependency. Thanks a lot for the info and let me know if my understanding is wrong :) -- Emre

1 0

[libvirt] Power Hypervisor: Fix potential segfault and memleak in phypOpen
by Matthias Bolte 20 Aug '09

20 Aug '09

Hi, I came across this line in the phypOpen function: char string[strlen(conn->uri->path)]; Here the path part of the given URI is used without checking it for NULL, this can cause a segfault as strlen expects a string != NULL. Beside that uuid_db and connection_data leak in case of an error. In this line conn->uri->path = string; the original path of the URI leaks. The patch adds a VIR_FREE call before setting the new path. The attached patch is compile-tested but I don't have a Power Hypervisor installation at hand to test it for real. Matthias

4 7

[libvirt] [PATCH] Introduce virStrcpy.
by Chris Lalancette 20 Aug '09

20 Aug '09

Add the virStrcpy function, which takes a dst string, source string, and number of bytes available in the dest string. If the source string is too large to fit into the destination string, including the \0 byte, then no data is copied and the function returns NULL. Otherwise, this function copies source into dst, including the \0, and returns a pointer to the dst string. This function is intended to replace all unsafe uses of strncpy in the code base, since strncpy does *not* guarantee that the buffer terminates with a \0. Signed-off-by: Chris Lalancette <clalance(a)redhat.com> --- cfg.mk | 5 ++ proxy/libvirt_proxy.c | 7 ++- qemud/Makefile.am | 3 +- qemud/qemud.c | 7 +- qemud/remote.c | 9 ++- src/bridge.c | 89 ++++++++-------------------- src/datatypes.c | 8 ++- src/esx/esx_driver.c | 10 ++- src/esx/esx_vmx.c | 7 ++- src/lxc_controller.c | 7 ++- src/lxc_driver.c | 13 ++++- src/nodeinfo.c | 5 +- src/opennebula/one_client.c | 5 +- src/openvz_conf.c | 33 +++++++--- src/phyp/phyp_driver.c | 2 +- src/proxy_internal.c | 5 +- src/qemu_conf.c | 16 +++--- src/qemu_driver.c | 13 +++- src/remote_internal.c | 20 +++++-- src/test.c | 15 ++++- src/uml_driver.c | 17 ++++-- src/util.c | 31 ++++++++-- src/util.h | 1 + src/xen_internal.c | 22 +++++-- src/xend_internal.c | 62 ++++++++++++++------ src/xm_internal.c | 134 +++++++++++++++++++++++++++--------------- 26 files changed, 343 insertions(+), 203 deletions(-) diff --git a/cfg.mk b/cfg.mk index 64bd26e..ad96bc6 100644 --- a/cfg.mk +++ b/cfg.mk @@ -91,6 +91,11 @@ sc_prohibit_asprintf: msg='use virAsprintf, not a'sprintf \ $(_prohibit_regexp) +sc_prohibit_strncpy: + @re='strncpy *\(' \ + msg='use virStrcpy, not strncpy' \ + $(_prohibit_regexp) + sc_prohibit_VIR_ERR_NO_MEMORY: @re='\<V''IR_ERR_NO_MEMORY\>' \ msg='use virReportOOMError, not V'IR_ERR_NO_MEMORY \ diff --git a/proxy/libvirt_proxy.c b/proxy/libvirt_proxy.c index e008a7f..42084c7 100644 --- a/proxy/libvirt_proxy.c +++ b/proxy/libvirt_proxy.c @@ -167,10 +167,13 @@ proxyListenUnixSocket(const char *path) { * Abstract socket do not hit the filesystem, way more secure and * guaranteed to be atomic */ - memset(&addr, 0, sizeof(addr)); addr.sun_family = AF_UNIX; addr.sun_path[0] = '\0'; - strncpy(&addr.sun_path[1], path, (sizeof(addr) - 4) - 2); + if (virStrcpy(&addr.sun_path[1], path, sizeof(addr.sun_path) - 1) == NULL) { + fprintf(stderr, "Path %s too long to fit into destination\n", path); + close(fd); + return -1; + } /* * now bind the socket to that address and listen on it diff --git a/qemud/Makefile.am b/qemud/Makefile.am index 5f1376e..3220ded 100644 --- a/qemud/Makefile.am +++ b/qemud/Makefile.am @@ -108,9 +108,10 @@ libvirtd_LDADD = \ $(SASL_LIBS) \ $(POLKIT_LIBS) +libvirtd_LDADD += ../src/libvirt_util.la + if WITH_DRIVER_MODULES libvirtd_LDADD += ../src/libvirt_driver.la - libvirtd_LDADD += ../src/libvirt_util.la else if WITH_QEMU libvirtd_LDADD += ../src/libvirt_driver_qemu.la diff --git a/qemud/qemud.c b/qemud/qemud.c index b2bd59d..17fc172 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -509,13 +509,14 @@ static int qemudListenUnix(struct qemud_server *server, virSetNonBlock(sock->fd) < 0) goto cleanup; - memset(&addr, 0, sizeof(addr)); addr.sun_family = AF_UNIX; - strncpy(addr.sun_path, path, sizeof(addr.sun_path)-1); + if (virStrcpy(addr.sun_path, path, sizeof(addr.sun_path)) == NULL) { + VIR_ERROR(_("Path %s too long for unix socket"), path); + goto cleanup; + } if (addr.sun_path[0] == '@') addr.sun_path[0] = '\0'; - oldgrp = getgid(); oldmask = umask(readonly ? ~unix_sock_ro_mask : ~unix_sock_rw_mask); if (server->privileged) diff --git a/qemud/remote.c b/qemud/remote.c index 2225d06..92cc925 100644 --- a/qemud/remote.c +++ b/qemud/remote.c @@ -570,9 +570,12 @@ remoteDispatchDomainSetSchedulerParameters (struct qemud_server *server ATTRIBUT /* Deserialise parameters. */ for (i = 0; i < nparams; ++i) { - strncpy (params[i].field, args->params.params_val[i].field, - VIR_DOMAIN_SCHED_FIELD_LENGTH); - params[i].field[VIR_DOMAIN_SCHED_FIELD_LENGTH-1] = '\0'; + if (virStrcpy(params[i].field, args->params.params_val[i].field, + sizeof(params[i].field)) == NULL) { + remoteDispatchFormatError(rerr, _("Field %s too big for destination"), + args->params.params_val[i].field); + return -1; + } params[i].type = args->params.params_val[i].value.type; switch (params[i].type) { case VIR_DOMAIN_SCHED_FIELD_INT: diff --git a/src/bridge.c b/src/bridge.c index 414d87b..378c055 100644 --- a/src/bridge.c +++ b/src/bridge.c @@ -149,23 +149,19 @@ brHasBridge(brControl *ctl, const char *name) { struct ifreq ifr; - int len; if (!ctl || !name) { errno = EINVAL; return -1; } - if ((len = strlen(name)) >= BR_IFNAME_MAXLEN) { + memset(&ifr, 0, sizeof(struct ifreq)); + + if (virStrcpy(ifr.ifr_name, name, sizeof(ifr.ifr_name)) == NULL) { errno = EINVAL; return -1; } - memset(&ifr, 0, sizeof(struct ifreq)); - - strncpy(ifr.ifr_name, name, len); - ifr.ifr_name[len] = '\0'; - if (ioctl(ctl->fd, SIOCGIFFLAGS, &ifr)) return -1; @@ -216,18 +212,14 @@ brAddDelInterface(brControl *ctl, const char *iface) { struct ifreq ifr; - int len; if (!ctl || !ctl->fd || !bridge || !iface) return EINVAL; - if ((len = strlen(bridge)) >= BR_IFNAME_MAXLEN) - return EINVAL; - memset(&ifr, 0, sizeof(struct ifreq)); - strncpy(ifr.ifr_name, bridge, len); - ifr.ifr_name[len] = '\0'; + if (virStrcpy(ifr.ifr_name, bridge, sizeof(ifr.ifr_name)) == NULL) + return EINVAL; if (!(ifr.ifr_ifindex = if_nametoindex(iface))) return ENODEV; @@ -305,23 +297,19 @@ brDeleteInterface(brControl *ctl ATTRIBUTE_UNUSED, static int ifGetMtu(brControl *ctl, const char *ifname) { struct ifreq ifr; - int len; if (!ctl || !ifname) { errno = EINVAL; return -1; } - if ((len = strlen(ifname)) >= BR_IFNAME_MAXLEN) { + memset(&ifr, 0, sizeof(struct ifreq)); + + if (virStrcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name)) == NULL) { errno = EINVAL; return -1; } - memset(&ifr, 0, sizeof(struct ifreq)); - - strncpy(ifr.ifr_name, ifname, len); - ifr.ifr_name[len] = '\0'; - if (ioctl(ctl->fd, SIOCGIFMTU, &ifr)) return -1; @@ -343,18 +331,14 @@ static int ifGetMtu(brControl *ctl, const char *ifname) static int ifSetMtu(brControl *ctl, const char *ifname, int mtu) { struct ifreq ifr; - int len; if (!ctl || !ifname) return EINVAL; - if ((len = strlen(ifname)) >= BR_IFNAME_MAXLEN) - return EINVAL; - memset(&ifr, 0, sizeof(struct ifreq)); - strncpy(ifr.ifr_name, ifname, len); - ifr.ifr_name[len] = '\0'; + if (virStrcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name)) == NULL) + return EINVAL; ifr.ifr_mtu = mtu; return ioctl(ctl->fd, SIOCSIFMTU, &ifr) == 0 ? 0 : errno; @@ -466,7 +450,7 @@ brAddTap(brControl *ctl, int vnet_hdr, int *tapfd) { - int fd, len; + int fd; struct ifreq ifr; if (!ctl || !ctl->fd || !bridge || !ifname) @@ -486,17 +470,14 @@ brAddTap(brControl *ctl, (void) vnet_hdr; #endif - strncpy(ifr.ifr_name, *ifname, IFNAMSIZ-1); - - if (ioctl(fd, TUNSETIFF, &ifr) < 0) - goto error; - - len = strlen(ifr.ifr_name); - if (len >= BR_IFNAME_MAXLEN - 1) { + if (virStrcpy(ifr.ifr_name, *ifname, sizeof(ifr.ifr_name)) == NULL) { errno = EINVAL; goto error; } + if (ioctl(fd, TUNSETIFF, &ifr) < 0) + goto error; + /* We need to set the interface MTU before adding it * to the bridge, because the bridge will have its * MTU adjusted automatically when we add the new interface. @@ -527,7 +508,6 @@ int brDeleteTap(brControl *ctl, const char *ifname) { struct ifreq try; - int len; int fd; if (!ctl || !ctl->fd || !ifname) @@ -539,15 +519,11 @@ int brDeleteTap(brControl *ctl, memset(&try, 0, sizeof(struct ifreq)); try.ifr_flags = IFF_TAP|IFF_NO_PI; - len = strlen(ifname); - if (len >= BR_IFNAME_MAXLEN - 1) { + if (virStrcpy(try.ifr_name, ifname, sizeof(try.ifr_name)) == NULL) { errno = EINVAL; goto error; } - strncpy(try.ifr_name, ifname, len); - try.ifr_name[len] = '\0'; - if (ioctl(fd, TUNSETIFF, &try) == 0) { if ((errno = ioctl(fd, TUNSETPERSIST, 0))) goto error; @@ -576,19 +552,15 @@ brSetInterfaceUp(brControl *ctl, int up) { struct ifreq ifr; - int len; int flags; if (!ctl || !ifname) return EINVAL; - if ((len = strlen(ifname)) >= BR_IFNAME_MAXLEN) - return EINVAL; - memset(&ifr, 0, sizeof(struct ifreq)); - strncpy(ifr.ifr_name, ifname, len); - ifr.ifr_name[len] = '\0'; + if (virStrcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name)) == NULL) + return EINVAL; if (ioctl(ctl->fd, SIOCGIFFLAGS, &ifr) < 0) return errno; @@ -621,18 +593,14 @@ brGetInterfaceUp(brControl *ctl, int *up) { struct ifreq ifr; - int len; if (!ctl || !ifname || !up) return EINVAL; - if ((len = strlen(ifname)) >= BR_IFNAME_MAXLEN) - return EINVAL; - memset(&ifr, 0, sizeof(struct ifreq)); - strncpy(ifr.ifr_name, ifname, len); - ifr.ifr_name[len] = '\0'; + if (virStrcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name)) == NULL) + return EINVAL; if (ioctl(ctl->fd, SIOCGIFFLAGS, &ifr) < 0) return errno; @@ -654,18 +622,15 @@ brSetInetAddr(brControl *ctl, } s; struct ifreq ifr; struct in_addr inaddr; - int len, ret; + int ret; if (!ctl || !ctl->fd || !ifname || !addr) return EINVAL; - if ((len = strlen(ifname)) >= BR_IFNAME_MAXLEN) - return EINVAL; - memset(&ifr, 0, sizeof(struct ifreq)); - strncpy(ifr.ifr_name, ifname, len); - ifr.ifr_name[len] = '\0'; + if (virStrcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name)) == NULL) + return EINVAL; if ((ret = inet_pton(AF_INET, addr, &inaddr)) < 0) return errno; @@ -692,18 +657,14 @@ brGetInetAddr(brControl *ctl, { struct ifreq ifr; struct in_addr *inaddr; - int len; if (!ctl || !ctl->fd || !ifname || !addr) return EINVAL; - if ((len = strlen(ifname)) >= BR_IFNAME_MAXLEN) - return EINVAL; - memset(&ifr, 0, sizeof(struct ifreq)); - strncpy(ifr.ifr_name, ifname, len); - ifr.ifr_name[len] = '\0'; + if (virStrcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name)) == NULL) + return EINVAL; if (ioctl(ctl->fd, cmd, &ifr) < 0) return errno; diff --git a/src/datatypes.c b/src/datatypes.c index ac61682..b10452e 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -25,6 +25,7 @@ #include "virterror_internal.h" #include "logging.h" #include "memory.h" +#include "util.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -876,8 +877,11 @@ virGetStorageVol(virConnectPtr conn, const char *pool, const char *name, const c virReportOOMError(conn); goto error; } - strncpy(ret->key, key, sizeof(ret->key)-1); - ret->key[sizeof(ret->key)-1] = '\0'; + if (virStrcpy(ret->key, key, sizeof(ret->key)) == NULL) { + virLibConnError(conn, VIR_ERR_INTERNAL_ERROR, + _("Volume key %s too large for destination"), key); + goto error; + } ret->magic = VIR_STORAGE_VOL_MAGIC; ret->conn = conn; diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index dba6305..c5dbda2 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -696,9 +696,13 @@ esxNodeGetInfo(virConnectPtr conn, virNodeInfoPtr nodeinfo) ++ptr; } - strncpy (nodeinfo->model, dynamicProperty->val->string, - sizeof (nodeinfo->model) - 1); - nodeinfo->model[sizeof (nodeinfo->model) - 1] = '\0'; + if (virStrcpy(nodeinfo->model, dynamicProperty->val->string, + sizeof(nodeinfo->model)) == NULL) { + ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, + "Model %s too long for destination", + dynamicProperty->val->string); + goto failure; + } } else { VIR_WARN("Unexpected '%s' property", dynamicProperty->name); } diff --git a/src/esx/esx_vmx.c b/src/esx/esx_vmx.c index 91a86e2..b138d13 100644 --- a/src/esx/esx_vmx.c +++ b/src/esx/esx_vmx.c @@ -885,8 +885,11 @@ esxVMX_IndexToDiskName(virConnectPtr conn, int idx, const char *prefix) return NULL; } - strncpy(buffer, prefix, sizeof (buffer) - 1); - buffer[sizeof (buffer) - 1] = '\0'; + if (virStrcpy(buffer, prefix, sizeof(buffer)) == NULL) { + ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, + "Prefix %s too big for destination", prefix); + return NULL; + } if (idx < 26) { buffer[length] = 'a' + idx; diff --git a/src/lxc_controller.c b/src/lxc_controller.c index 8d11238..6bc9976 100644 --- a/src/lxc_controller.c +++ b/src/lxc_controller.c @@ -176,9 +176,12 @@ static int lxcMonitorServer(const char *sockpath) } unlink(sockpath); - memset(&addr, 0, sizeof(addr)); addr.sun_family = AF_UNIX; - strncpy(addr.sun_path, sockpath, sizeof(addr.sun_path)); + if (virStrcpy(addr.sun_path, sockpath, sizeof(addr.sun_path)) == NULL) { + lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Socket path %s too long for destination"), sockpath); + goto error; + } if (bind(fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) { virReportSystemError(NULL, errno, diff --git a/src/lxc_driver.c b/src/lxc_driver.c index c6cf935..b42f77f 100644 --- a/src/lxc_driver.c +++ b/src/lxc_driver.c @@ -719,7 +719,11 @@ static int lxcMonitorClient(virConnectPtr conn, memset(&addr, 0, sizeof(addr)); addr.sun_family = AF_UNIX; - strncpy(addr.sun_path, sockpath, sizeof(addr.sun_path)); + if (virStrcpy(addr.sun_path, sockpath, sizeof(addr.sun_path)) == NULL) { + lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR, + _("Socket path %s too big for destination"), sockpath); + goto error; + } if (connect(fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) { virReportSystemError(conn, errno, "%s", @@ -1737,7 +1741,12 @@ static int lxcGetSchedulerParameters(virDomainPtr domain, if (virCgroupGetCpuShares(group, &val) != 0) goto cleanup; params[0].value.ul = val; - strncpy(params[0].field, "cpu_shares", sizeof(params[0].field)); + if (virStrcpy(params[0].field, "cpu_shares", + sizeof(params[0].field)) == NULL) { + lxcError(NULL, domain, VIR_ERR_INTERNAL_ERROR, + "%s", _("Field cpu_shares too big for destination")); + goto cleanup; + } params[0].type = VIR_DOMAIN_SCHED_FIELD_ULLONG; ret = 0; diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 0a211a7..b620ced 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -141,8 +141,9 @@ int nodeGetInfo(virConnectPtr conn, uname(&info); - strncpy(nodeinfo->model, info.machine, sizeof(nodeinfo->model)-1); - nodeinfo->model[sizeof(nodeinfo->model)-1] = '\0'; + if (virStrcpy(nodeinfo->model, info.machine, + sizeof(nodeinfo->model)) == NULL) + return -1; #else /* !HAVE_UNAME */ diff --git a/src/opennebula/one_client.c b/src/opennebula/one_client.c index 21f303a..9d3a5c6 100644 --- a/src/opennebula/one_client.c +++ b/src/opennebula/one_client.c @@ -24,6 +24,8 @@ #include <fcntl.h> #include <unistd.h> #include "one_client.h" +#include "datatypes.h" +#include "util.h" oneClient one_client; @@ -186,8 +188,7 @@ int c_oneVmInfo(int vmid, char* ret_info,int length) if( return_code ) { - strncpy(ret_info, return_string, length-1); - ret_info[length-1] = '\0'; + virStrcpy(ret_info, return_string, length); retval = 0; } diff --git a/src/openvz_conf.c b/src/openvz_conf.c index a172fe3..27f0264 100644 --- a/src/openvz_conf.c +++ b/src/openvz_conf.c @@ -261,8 +261,11 @@ openvzReadNetworkConf(virConnectPtr conn, if (VIR_ALLOC_N(net->ifname, len+1) < 0) goto no_memory; - strncpy(net->ifname, p, len); - net->ifname[len] = '\0'; + if (virStrcpy(net->ifname, p, len+1) == NULL) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + _("Network ifname %s too long for destination"), p); + goto error; + } } else if (STRPREFIX(p, "bridge=")) { p += 7; len = next - p; @@ -275,8 +278,11 @@ openvzReadNetworkConf(virConnectPtr conn, if (VIR_ALLOC_N(net->data.bridge.brname, len+1) < 0) goto no_memory; - strncpy(net->data.bridge.brname, p, len); - net->data.bridge.brname[len] = '\0'; + if (virStrcpy(net->data.bridge.brname, p, len+1) == NULL) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + _("Bridge name %s too long for destination"), p); + goto error; + } } else if (STRPREFIX(p, "mac=")) { p += 4; len = next - p; @@ -285,8 +291,11 @@ openvzReadNetworkConf(virConnectPtr conn, "%s", _("Wrong length MAC address")); goto error; } - strncpy(cpy_temp, p, len); - cpy_temp[len] = '\0'; + if (virStrcpy(cpy_temp, p, sizeof(cpy_temp)) == NULL) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, + _("MAC address %s too long for destination"), p); + goto error; + } if (virParseMacAddr(cpy_temp, net->mac) < 0) { openvzError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("Wrong MAC address")); @@ -629,8 +638,10 @@ openvzReadConfigParam(const char * conf_file ,const char * param, char *value, i if (sf[0] == '=' && sf[1] != '\0' ) { sf ++; if ((token = strtok_r(sf,"\"\t\n", &saveptr)) != NULL) { - strncpy(value, token, maxlen) ; - value[maxlen-1] = '\0'; + if (virStrcpy(value, token, maxlen) == NULL) { + ret = -1; + break; + } found = 1; } } @@ -812,6 +823,7 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len) char uuidbuf[1024]; char iden[1024]; int fd, ret; + int retval = 0; if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX, "conf")<0) return -1; @@ -832,13 +844,14 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len) sscanf(line, "%s %s\n", iden, uuidbuf); if(STREQ(iden, "#UUID:")) { - strncpy(uuidstr, uuidbuf, len); + if (virStrcpy(uuidstr, uuidbuf, len) == NULL) + retval = -1; break; } } close(fd); - return 0; + return retval; } /* Do actual checking for UUID presence in conf file, diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index f457cf4..508767e 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1369,7 +1369,7 @@ escape_specialcharacters(char *src, char *dst, size_t dstlen) } temp_buffer[j] = '\0'; - if (strncpy(dst, temp_buffer, dstlen) == NULL) + if (virStrcpy(dst, temp_buffer, dstlen) == NULL) return -1; return 0; diff --git a/src/proxy_internal.c b/src/proxy_internal.c index 5b92ad8..8609201 100644 --- a/src/proxy_internal.c +++ b/src/proxy_internal.c @@ -197,7 +197,10 @@ retry: memset(&addr, 0, sizeof(addr)); addr.sun_family = AF_UNIX; addr.sun_path[0] = '\0'; - strncpy(&addr.sun_path[1], path, (sizeof(addr) - 4) - 2); + if (virStrcpy(&addr.sun_path[1], path, sizeof(addr.sun_path) - 1) == NULL) { + close(fd); + return -1; + } /* * now bind the socket to that address and listen on it diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 6b0b404..4690c69 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1282,18 +1282,18 @@ static int qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev, { switch (dev->type) { case VIR_DOMAIN_CHR_TYPE_NULL: - strncpy(buf, "null", buflen); - buf[buflen-1] = '\0'; + if (virStrcpy(buf, "null", buflen) == NULL) + return -1; break; case VIR_DOMAIN_CHR_TYPE_VC: - strncpy(buf, "vc", buflen); - buf[buflen-1] = '\0'; + if (virStrcpy(buf, "vc", buflen) == NULL) + return -1; break; case VIR_DOMAIN_CHR_TYPE_PTY: - strncpy(buf, "pty", buflen); - buf[buflen-1] = '\0'; + if (virStrcpy(buf, "pty", buflen) == NULL) + return -1; break; case VIR_DOMAIN_CHR_TYPE_DEV: @@ -1315,8 +1315,8 @@ static int qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev, break; case VIR_DOMAIN_CHR_TYPE_STDIO: - strncpy(buf, "stdio", buflen); - buf[buflen-1] = '\0'; + if (virStrcpy(buf, "stdio", buflen) == NULL) + return -1; break; case VIR_DOMAIN_CHR_TYPE_UDP: diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 37fdec2..bcacd41 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -909,7 +909,11 @@ qemudOpenMonitorUnix(virConnectPtr conn, memset(&addr, 0, sizeof(addr)); addr.sun_family = AF_UNIX; - strncpy(addr.sun_path, monitor, sizeof(addr.sun_path)); + if (virStrcpy(addr.sun_path, monitor, sizeof(addr.sun_path)) == NULL) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Monitor path %s too big for destination"), monitor); + goto error; + } do { ret = connect(monfd, (struct sockaddr *) &addr, sizeof(addr)); @@ -5802,8 +5806,11 @@ static int qemuGetSchedulerParameters(virDomainPtr dom, goto cleanup; } params[0].value.ul = val; - strncpy(params[0].field, "cpu_shares", sizeof(params[0].field)); - params[0].type = VIR_DOMAIN_SCHED_FIELD_ULLONG; + if (virStrcpy(params[0].field, "cpu_shares", sizeof(params[0].field)) == NULL) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, + "%s", _("Field cpu_shares too long for destination")); + goto cleanup; + } ret = 0; diff --git a/src/remote_internal.c b/src/remote_internal.c index ad0cbf0..bc69c70 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -670,7 +670,11 @@ doRemoteOpen (virConnectPtr conn, memset (&addr, 0, sizeof addr); addr.sun_family = AF_UNIX; - strncpy (addr.sun_path, sockname, UNIX_PATH_MAX (addr)); + if (virStrcpy(addr.sun_path, sockname, sizeof(addr.sun_path)) == NULL) { + errorf(conn, VIR_ERR_INTERNAL_ERROR, + _("Socket %s too big for destination"), sockname); + goto failed; + } if (addr.sun_path[0] == '@') addr.sun_path[0] = '\0'; @@ -1585,8 +1589,8 @@ remoteNodeGetInfo (virConnectPtr conn, virNodeInfoPtr info) (xdrproc_t) xdr_remote_node_get_info_ret, (char *) &ret) == -1) goto done; - strncpy (info->model, ret.model, 32); - info->model[31] = '\0'; + if (virStrcpy(info->model, ret.model, sizeof(info->model)) == NULL) + goto done; info->memory = ret.memory; info->cpus = ret.cpus; info->mhz = ret.mhz; @@ -3017,9 +3021,13 @@ remoteDomainGetSchedulerParameters (virDomainPtr domain, /* Deserialise the result. */ for (i = 0; i < *nparams; ++i) { - strncpy (params[i].field, ret.params.params_val[i].field, - VIR_DOMAIN_SCHED_FIELD_LENGTH); - params[i].field[VIR_DOMAIN_SCHED_FIELD_LENGTH-1] = '\0'; + if (virStrcpy(params[i].field, ret.params.params_val[i].field, + sizeof(params[i].field)) == NULL) { + errorf(domain->conn, VIR_ERR_INTERNAL_ERROR, + _("Parameter %s too big for destination"), + ret.params.params_val[i].field); + goto cleanup; + } params[i].type = ret.params.params_val[i].value.type; switch (params[i].type) { case VIR_DOMAIN_SCHED_FIELD_INT: diff --git a/src/test.c b/src/test.c index ab6cfdf..e265d38 100644 --- a/src/test.c +++ b/src/test.c @@ -570,8 +570,11 @@ static int testOpenFromFile(virConnectPtr conn, privconn->nextDomID = 1; privconn->numCells = 0; - strncpy(privconn->path, file, PATH_MAX-1); - privconn->path[PATH_MAX-1] = '\0'; + if (virStrcpy(privconn->path, file, sizeof(privconn->path)) == NULL) { + testError(NULL, VIR_ERR_INTERNAL_ERROR, + _("Path %s too big for destination"), file); + goto error; + } memmove(&privconn->nodeInfo, &defaultNodeInfo, sizeof(defaultNodeInfo)); nodeInfo = &privconn->nodeInfo; @@ -627,8 +630,12 @@ static int testOpenFromFile(virConnectPtr conn, str = virXPathString(conn, "string(/node/cpu/model[1])", ctxt); if (str != NULL) { - strncpy(nodeInfo->model, str, sizeof(nodeInfo->model)-1); - nodeInfo->model[sizeof(nodeInfo->model)-1] = '\0'; + if (virStrcpy(nodeInfo->model, str, sizeof(nodeInfo->model)) == NULL) { + testError(NULL, VIR_ERR_INTERNAL_ERROR, + _("Model %s too big for destination"), str); + VIR_FREE(str); + goto error; + } VIR_FREE(str); } diff --git a/src/uml_driver.c b/src/uml_driver.c index 53e7e10..815a4b3 100644 --- a/src/uml_driver.c +++ b/src/uml_driver.c @@ -562,6 +562,7 @@ static int umlMonitorAddress(virConnectPtr conn, virDomainObjPtr vm, struct sockaddr_un *addr) { char *sockname; + int retval = 0; if (virAsprintf(&sockname, "%s/%s/mconsole", driver->monitorDir, vm->def->name) < 0) { @@ -571,10 +572,13 @@ static int umlMonitorAddress(virConnectPtr conn, memset(addr, 0, sizeof *addr); addr->sun_family = AF_UNIX; - strncpy(addr->sun_path, sockname, sizeof(addr->sun_path)-1); - NUL_TERMINATE(addr->sun_path); + if (virStrcpy(addr->sun_path, sockname, sizeof(addr->sun_path)) == NULL) { + umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Unix path %s too long for destination"), sockname); + retval = -1; + } VIR_FREE(sockname); - return 0; + return retval; } static int umlOpenMonitor(virConnectPtr conn, @@ -668,8 +672,11 @@ static int umlMonitorCommand(virConnectPtr conn, cmd, req.length); return -1; } - strncpy(req.data, cmd, req.length); - req.data[req.length] = '\0'; + if (virStrcpy(req.data, cmd, sizeof(req.data)) == NULL) { + umlReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("Command %s too long for destination"), cmd); + return -1; + } if (sendto(vm->monitor, &req, sizeof req, 0, (struct sockaddr *)&addr, sizeof addr) != (sizeof req)) { diff --git a/src/util.c b/src/util.c index 0d4f3fa..0d252e6 100644 --- a/src/util.c +++ b/src/util.c @@ -1121,9 +1121,8 @@ char *virFindFileInPath(const char *file) char fullpath[PATH_MAX]; /* copy PATH env so we can tweak it */ - strncpy(pathenv, getenv("PATH"), PATH_MAX); - pathenv[PATH_MAX - 1] = '\0'; - + if (virStrcpy(pathenv, getenv("PATH"), sizeof(pathenv)) == NULL) + return NULL; /* for each path segment, append the file to search for and test for * it. return it if found. @@ -1155,8 +1154,8 @@ int virFileMakePath(const char *path) if (stat(path, &st) >= 0) return 0; - strncpy(parent, path, PATH_MAX); - parent[PATH_MAX - 1] = '\0'; + if (virStrcpy(parent, path, sizeof(parent)) == NULL) + return EINVAL; if (!(p = strrchr(parent, '/'))) return EINVAL; @@ -1573,6 +1572,28 @@ virAsprintf(char **strp, const char *fmt, ...) return ret; } +/** + * virStrcpy + * + * A safe version of strcpy. The last parameter is the number of bytes + * available in the destination string, *not* the number of bytes you want + * to copy. If the destination is not large enough to hold all of the + * src string, NULL is returned and no data is copied. If the destination + * is large enough to hold all of the src, then the string is copied and + * a pointer to the destination string is returned. + */ +char * +virStrcpy(char *dest, const char *src, size_t destbytes) +{ + int len; + + len = strlen(src) + 1; + if (len > destbytes) + return NULL; + + return memcpy(dest, src, len); +} + /* Compare two MAC addresses, ignoring differences in case, * as well as leading zeros. */ diff --git a/src/util.h b/src/util.h index b3e628a..ffbf2b3 100644 --- a/src/util.h +++ b/src/util.h @@ -164,6 +164,7 @@ void virSkipSpaces(const char **str); int virParseNumber(const char **str); int virAsprintf(char **strp, const char *fmt, ...) ATTRIBUTE_FMT_PRINTF(2, 3); +char *virStrcpy(char *dest, const char *src, size_t destbytes); #define VIR_MAC_BUFLEN 6 #define VIR_MAC_PREFIX_BUFLEN 3 diff --git a/src/xen_internal.c b/src/xen_internal.c index ae78f84..fcc4977 100644 --- a/src/xen_internal.c +++ b/src/xen_internal.c @@ -1208,13 +1208,21 @@ xenHypervisorGetSchedulerParameters(virDomainPtr domain, if (ret < 0) return(-1); - strncpy (params[0].field, str_weight, VIR_DOMAIN_SCHED_FIELD_LENGTH); - params[0].field[VIR_DOMAIN_SCHED_FIELD_LENGTH-1] = '\0'; + if (virStrcpy(params[0].field, str_weight, + sizeof(params[0].field)) == NULL) { + virXenError (domain->conn, VIR_ERR_INTERNAL_ERROR, + "Weight %s too big for destination", str_weight); + return -1; + } params[0].type = VIR_DOMAIN_SCHED_FIELD_UINT; params[0].value.ui = op_dom.u.getschedinfo.u.credit.weight; - strncpy (params[1].field, str_cap, VIR_DOMAIN_SCHED_FIELD_LENGTH); - params[1].field[VIR_DOMAIN_SCHED_FIELD_LENGTH-1] = '\0'; + if (virStrcpy(params[1].field, str_cap, + sizeof(params[1].field)) == NULL) { + virXenError (domain->conn, VIR_ERR_INTERNAL_ERROR, + "Cap %s too big for destination", str_cap); + return -1; + } params[1].type = VIR_DOMAIN_SCHED_FIELD_UINT; params[1].value.ui = op_dom.u.getschedinfo.u.credit.cap; @@ -2431,9 +2439,9 @@ xenHypervisorMakeCapabilitiesInternal(virConnectPtr conn, while (fgets (line, sizeof line, cpuinfo)) { if (regexec (&flags_hvm_rec, line, sizeof(subs)/sizeof(regmatch_t), subs, 0) == 0 && subs[0].rm_so != -1) { - strncpy (hvm_type, - &line[subs[1].rm_so], subs[1].rm_eo-subs[1].rm_so+1); - hvm_type[subs[1].rm_eo-subs[1].rm_so] = '\0'; + if (virStrcpy(hvm_type, &line[subs[1].rm_so], + sizeof(hvm_type)) == NULL) + return NULL; } else if (regexec (&flags_pae_rec, line, 0, NULL, 0) == 0) host_pae = 1; } diff --git a/src/xend_internal.c b/src/xend_internal.c index 7bcee7d..a7167ae 100644 --- a/src/xend_internal.c +++ b/src/xend_internal.c @@ -763,7 +763,8 @@ xenDaemonOpen_unix(virConnectPtr conn, const char *path) addr = (struct sockaddr_un *)&priv->addr; addr->sun_family = AF_UNIX; memset(addr->sun_path, 0, sizeof(addr->sun_path)); - strncpy(addr->sun_path, path, sizeof(addr->sun_path)); + if (virStrcpy(addr->sun_path, path, sizeof(addr->sun_path)) == NULL) + return -1; return (0); } @@ -1442,8 +1443,11 @@ xenDaemonParseSxprChar(virConnectPtr conn, return NULL; } - strncpy(prefix, value, sizeof(prefix)-1); - NUL_TERMINATE(prefix); + if (virStrcpy(prefix, value, sizeof(prefix)) == NULL) { + virXendError(conn, VIR_ERR_INTERNAL_ERROR, + _("Value %s too big for prefix"), value); + goto error; + } if (value[0] == '/') { def->type = VIR_DOMAIN_CHR_TYPE_DEV; @@ -1647,8 +1651,12 @@ xenDaemonParseSxprDisks(virConnectPtr conn, if (VIR_ALLOC_N(disk->driverName, (offset-src)+1) < 0) goto no_memory; - strncpy(disk->driverName, src, (offset-src)); - disk->driverName[offset-src] = '\0'; + if (virStrcpy(disk->driverName, src, (offset-src)+1) == NULL) { + virXendError(conn, VIR_ERR_INTERNAL_ERROR, + _("Driver name %s too big for destination"), + src); + goto error; + } src = offset + 1; @@ -1662,8 +1670,12 @@ xenDaemonParseSxprDisks(virConnectPtr conn, if (VIR_ALLOC_N(disk->driverType, (offset-src)+1)< 0) goto no_memory; - strncpy(disk->driverType, src, (offset-src)); - disk->driverType[offset-src] = '\0'; + if (virStrcpy(disk->driverType, src, (offset-src)+1) == NULL) { + virXendError(conn, VIR_ERR_INTERNAL_ERROR, + _("Driver type %s too big for destination"), + src); + goto error; + } src = offset + 1; /* Its possible to use blktap driver for block devs @@ -1896,8 +1908,12 @@ xenDaemonParseSxprSound(virConnectPtr conn, _("unexpected sound model %s"), offset); goto error; } - strncpy(model, offset, len); - model[len] = '\0'; + if (virStrcpy(model, offset, sizeof(model)) == NULL) { + virXendError(conn, VIR_ERR_INTERNAL_ERROR, + _("Sound model %s too big for destination"), + offset); + goto error; + } if (VIR_ALLOC(sound) < 0) goto no_memory; @@ -4786,13 +4802,22 @@ xenDaemonGetSchedulerParameters(virDomainPtr domain, goto error; } - strncpy (params[0].field, str_weight, VIR_DOMAIN_SCHED_FIELD_LENGTH); - params[0].field[VIR_DOMAIN_SCHED_FIELD_LENGTH-1] = '\0'; + if (virStrcpy(params[0].field, str_weight, + sizeof(params[0].field)) == NULL) { + virXendError(domain->conn, VIR_ERR_INTERNAL_ERROR, + _("Weight %s too big for destination"), + str_weight); + goto error; + } params[0].type = VIR_DOMAIN_SCHED_FIELD_UINT; params[0].value.ui = sexpr_int(root, "domain/cpu_weight"); - strncpy (params[1].field, str_cap, VIR_DOMAIN_SCHED_FIELD_LENGTH); - params[1].field[VIR_DOMAIN_SCHED_FIELD_LENGTH-1] = '\0'; + if (virStrcpy(params[1].field, str_cap, + sizeof(params[1].field)) == NULL) { + virXendError(domain->conn, VIR_ERR_INTERNAL_ERROR, + _("Cap %s too big for destination"), str_cap); + goto error; + } params[1].type = VIR_DOMAIN_SCHED_FIELD_UINT; params[1].value.ui = sexpr_int(root, "domain/cpu_cap"); *nparams = XEN_SCHED_CRED_NPARAM; @@ -5842,6 +5867,7 @@ virDomainXMLDevID(virDomainPtr domain, { xenUnifiedPrivatePtr priv = domain->conn->privateData; char *xref; + char *tmp; if (dev->type == VIR_DOMAIN_DEVICE_DISK) { if (dev->data.disk->driverName && @@ -5859,9 +5885,10 @@ virDomainXMLDevID(virDomainPtr domain, if (xref == NULL) return -1; - strncpy(ref, xref, ref_len); + tmp = virStrcpy(ref, xref, ref_len); free(xref); - ref[ref_len - 1] = '\0'; + if (tmp == NULL) + return -1; } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { char mac[30]; virDomainNetDefPtr def = dev->data.net; @@ -5878,9 +5905,10 @@ virDomainXMLDevID(virDomainPtr domain, if (xref == NULL) return -1; - strncpy(ref, xref, ref_len); + tmp = virStrcpy(ref, xref, ref_len); free(xref); - ref[ref_len - 1] = '\0'; + if (tmp == NULL) + return -1; } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV && dev->data.hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && dev->data.hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) { diff --git a/src/xm_internal.c b/src/xm_internal.c index 71b852e..00d9e9a 100644 --- a/src/xm_internal.c +++ b/src/xm_internal.c @@ -872,8 +872,12 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { } else { if (VIR_ALLOC_N(disk->src, (offset - head) + 1) < 0) goto no_memory; - strncpy(disk->src, head, (offset - head)); - disk->src[(offset-head)] = '\0'; + if (virStrcpy(disk->src, head, (offset - head) + 1) == NULL) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("Source file %s too big for destination"), + head); + goto cleanup; + } } head = offset + 1; @@ -886,8 +890,11 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { goto skipdisk; if (VIR_ALLOC_N(disk->dst, (offset - head) + 1) < 0) goto no_memory; - strncpy(disk->dst, head, (offset - head)); - disk->dst[(offset-head)] = '\0'; + if (virStrcpy(disk->dst, head, (offset - head) + 1) == NULL) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("Dest file %s too big for destination"), head); + goto cleanup; + } head = offset + 1; @@ -897,8 +904,13 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { if ((tmp = strchr(disk->src, ':')) != NULL) { if (VIR_ALLOC_N(disk->driverName, (tmp - disk->src) + 1) < 0) goto no_memory; - strncpy(disk->driverName, disk->src, (tmp - disk->src)); - disk->driverName[tmp - disk->src] = '\0'; + if (virStrcpy(disk->driverName, disk->src, + (tmp - disk->src) + 1) == NULL) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("Driver name %s too big for destination"), + disk->src); + goto cleanup; + } /* Strip the prefix we found off the source file name */ memmove(disk->src, disk->src+(tmp-disk->src)+1, @@ -912,8 +924,13 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { goto skipdisk; if (VIR_ALLOC_N(disk->driverType, (tmp - disk->src) + 1) < 0) goto no_memory; - strncpy(disk->driverType, disk->src, (tmp - disk->src)); - disk->driverType[tmp - disk->src] = '\0'; + if (virStrcpy(disk->driverType, disk->src, + (tmp - disk->src) + 1) == NULL) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("Driver type %s too big for destination"), + disk->src); + goto cleanup; + } /* Strip the prefix we found off the source file name */ memmove(disk->src, disk->src+(tmp-disk->src)+1, @@ -1026,42 +1043,46 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { data++; if (STRPREFIX(key, "mac=")) { - int len = nextkey ? (nextkey - data) : 17; - if (len > 17) - len = 17; - strncpy(mac, data, len); - mac[len] = '\0'; + if (virStrcpy(mac, data, sizeof(mac)) == NULL) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("MAC address %s too big for destination"), + data); + goto cleanup; + } } else if (STRPREFIX(key, "bridge=")) { - int len = nextkey ? (nextkey - data) : sizeof(bridge)-1; + if (virStrcpy(bridge, data, sizeof(bridge)) == NULL) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("Bridge %s too big for destination"), + data); + goto cleanup; + } type = 1; - if (len > (sizeof(bridge)-1)) - len = sizeof(bridge)-1; - strncpy(bridge, data, len); - bridge[len] = '\0'; } else if (STRPREFIX(key, "script=")) { - int len = nextkey ? (nextkey - data) : PATH_MAX-1; - if (len > (PATH_MAX-1)) - len = PATH_MAX-1; - strncpy(script, data, len); - script[len] = '\0'; + if (virStrcpy(script, data, sizeof(script)) == NULL) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("Script %s too big for destination"), + data); + goto cleanup; + } } else if (STRPREFIX(key, "model=")) { - int len = nextkey ? (nextkey - data) : sizeof(model)-1; - if (len > (sizeof(model)-1)) - len = sizeof(model)-1; - strncpy(model, data, len); - model[len] = '\0'; + if (virStrcpy(model, data, sizeof(model)) == NULL) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("Model %s too big for destination"), data); + goto cleanup; + } } else if (STRPREFIX(key, "vifname=")) { - int len = nextkey ? (nextkey - data) : sizeof(vifname)-1; - if (len > (sizeof(vifname)-1)) - len = sizeof(vifname)-1; - strncpy(vifname, data, len); - vifname[len] = '\0'; + if (virStrcpy(vifname, data, sizeof(vifname)) == NULL) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("Vifname %s too big for destination"), + data); + goto cleanup; + } } else if (STRPREFIX(key, "ip=")) { - int len = nextkey ? (nextkey - data) : 15; - if (len > 15) - len = 15; - strncpy(ip, data, len); - ip[len] = '\0'; + if (virStrcpy(ip, data, sizeof(ip)) == NULL) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("IP %s too big for destination"), data); + goto cleanup; + } } while (nextkey && (nextkey[0] == ',' || @@ -1168,29 +1189,41 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { if ((nextkey - key) > (sizeof(domain)-1)) goto skippci; - strncpy(domain, key, sizeof(domain)); - domain[sizeof(domain)-1] = '\0'; + if (virStrcpy(domain, key, sizeof(domain)) == NULL) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("Domain %s too big for destination"), key); + goto cleanup; + } key = nextkey + 1; if (!(nextkey = strchr(key, ':'))) goto skippci; - strncpy(bus, key, sizeof(bus)); - bus[sizeof(bus)-1] = '\0'; + if (virStrcpy(bus, key, sizeof(bus)) == NULL) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("Bus %s too big for destination"), key); + goto cleanup; + } key = nextkey + 1; if (!(nextkey = strchr(key, '.'))) goto skippci; - strncpy(slot, key, sizeof(slot)); - slot[sizeof(slot)-1] = '\0'; + if (virStrcpy(slot, key, sizeof(slot)) == NULL) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("Slot %s too big for destination"), key); + goto cleanup; + } key = nextkey + 1; if (strlen(key) != 1) goto skippci; - strncpy(func, key, sizeof(func)); - func[sizeof(func)-1] = '\0'; + if (virStrcpy(func, key, sizeof(func)) == NULL) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("Function %s too big for destination"), key); + goto cleanup; + } if (virStrToLong_i(domain, NULL, 16, &domainID) < 0) goto skippci; @@ -1301,8 +1334,13 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { list->list->str) { char vfb[MAX_VFB]; char *key = vfb; - strncpy(vfb, list->list->str, MAX_VFB-1); - vfb[MAX_VFB-1] = '\0'; + + if (virStrcpy(vfb, list->list->str, sizeof(vfb)) == NULL) { + xenXMError(conn, VIR_ERR_INTERNAL_ERROR, + _("VFB %s too big for destination"), + list->list->str); + goto cleanup; + } if (VIR_ALLOC(graphics) < 0) goto no_memory; -- 1.6.0.6

2 2

[libvirt] libvirt with libssh2?
by Emre Erenoglu 20 Aug '09

20 Aug '09

Hi, I'm the package maintainer of libvirt for the Pardus distribution. While trying to update the package to the latest 0.7.0, I recognized that configure script does not find libssh. I checked our system and found out that we are using libssh2 instead of libssh. >From this website (http://www.libssh2.org/libssh2-vs-libssh.html) libssh2 developers summarize their differences. My questions: 1) Is there a specific reason why we are using libssh? Is there a patch to make libvirt use libssh2 instead? 2) What functionality do we loose if we don't have libssh in the system? (depending on this, I'll also package libssh or not) Thanks a lot for any comments, Best regards, Emre Erenoglu

2 1

[libvirt] [PATCH] Small fixes for qemu save compression.
by Chris Lalancette 20 Aug '09

20 Aug '09

Fix up a small memory leak pointed out by DanB; I was forgetting to release memory allocated to driver->saveImageFormat. Also add the "save_image_format" and "security" entries to the augeas lens. Signed-off-by: Chris Lalancette <clalance(a)redhat.com> --- qemud/libvirtd_qemu.aug | 2 ++ qemud/test_libvirtd_qemu.aug | 10 +++++++++- src/qemu_driver.c | 1 + 3 files changed, 12 insertions(+), 1 deletions(-) diff --git a/qemud/libvirtd_qemu.aug b/qemud/libvirtd_qemu.aug index 8b8aab2..2175e14 100644 --- a/qemud/libvirtd_qemu.aug +++ b/qemud/libvirtd_qemu.aug @@ -29,10 +29,12 @@ module Libvirtd_qemu = | str_entry "vnc_password" | bool_entry "vnc_sasl" | str_entry "vnc_sasl_dir" + | str_entry "security_driver" | str_entry "user" | str_entry "group" | str_array_entry "cgroup_controllers" | str_array_entry "cgroup_device_acl" + | str_entry "save_image_format" (* Each enty in the config is one of the following three ... *) let entry = vnc_entry diff --git a/qemud/test_libvirtd_qemu.aug b/qemud/test_libvirtd_qemu.aug index 274c89d..72f2227 100644 --- a/qemud/test_libvirtd_qemu.aug +++ b/qemud/test_libvirtd_qemu.aug @@ -80,6 +80,8 @@ vnc_sasl = 1 # vnc_sasl_dir = \"/some/directory/sasl2\" +security_driver = \"selinux\" + user = \"root\" group = \"root\" @@ -87,6 +89,8 @@ group = \"root\" cgroup_controllers = [ \"cpu\", \"devices\" ] cgroup_device_acl = [ \"/dev/null\", \"/dev/full\", \"/dev/zero\" ] + +save_image_format = \"gzip\" " test Libvirtd_qemu.lns get conf = @@ -170,7 +174,9 @@ cgroup_device_acl = [ \"/dev/null\", \"/dev/full\", \"/dev/zero\" ] { "#comment" = "" } { "vnc_sasl_dir" = "/some/directory/sasl2" } { "#empty" } -{ "user"= "root" } +{ "security_driver" = "selinux" } +{ "#empty" } +{ "user" = "root" } { "#empty" } { "group" = "root" } { "#empty" } @@ -184,3 +190,5 @@ cgroup_device_acl = [ \"/dev/null\", \"/dev/full\", \"/dev/zero\" ] { "2" = "/dev/full" } { "3" = "/dev/zero" } } +{ "#empty" } +{ "save_image_format" = "gzip" } diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 86a33f8..ffe9c16 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -666,6 +666,7 @@ qemudShutdown(void) VIR_FREE(qemu_driver->vncListen); VIR_FREE(qemu_driver->vncPassword); VIR_FREE(qemu_driver->vncSASLdir); + VIR_FREE(qemu_driver->saveImageFormat); /* Free domain callback list */ virDomainEventCallbackListFree(qemu_driver->domainEventCallbacks); -- 1.6.0.6

2 2

[libvirt] [PATCH 0/1] Multipath pool support
by David Allan 20 Aug '09

20 Aug '09

The following patch implements multipath pool support. It's very basic functionality, consisting of creating a pool that contains all the multipath devices on the host. That will cover the common case of users who just want to discover all the available multipath devices and assign them to guests. It doesn't currently allow configuration of multipathing, so for now setting the multipath configuration will have to continue to be done as part of the host system build. Example XML to create the pool is: <pool type="mpath"> <name>mpath</name> <target> <path>/dev/mapper</path> </target> </pool> The target element is ignored, as it is by the disk pool, but the config code rejects the XML if it does not exist. That behavior should obviously be cleaned up, but I think that should be done in a separate patch, as it's really a bug in the config code, not related to the addition of the new pool type. Dave

4 8

[libvirt] FYI: Updated QEMU driver docs on security model
by Daniel P. Berrange 20 Aug '09

20 Aug '09

FYI, I just pushed the following patch to the repo which adds documentation to the website for all the security model related aspects of libvirt's QEMU driver. It should appear here shortly http://libvirt.org/drvqemu.html Regards, Daniel diff --git a/docs/drvqemu.html.in b/docs/drvqemu.html.in index e4c3d03..348eaaf 100644 --- a/docs/drvqemu.html.in +++ b/docs/drvqemu.html.in @@ -54,6 +54,292 @@ qemu+ssh://root@example.com/system (remote access, SSH tunnelled) </pre> + <h2><a name="security">Driver security architecture</a></h2> + + <p> + There are multiple layers to security in the QEMU driver, allowing for + flexibility in the use of QEMU based virtual machines. + </p> + + <h3><a name="securitydriver">Driver instances</a></h3> + + <p> + As explained above there are two ways to access the QEMU driver + in libvirt. The "qemu:///session" family of URIs connect to a + libvirtd instance running as the same user/group ID as the client + application. Thus the QEMU instances spawned from this driver will + share the same privileges as the client application. The intended + use case for this driver is desktop virtualization, with virtual + machines storing their disk imags in the user's home directory and + being managed from the local desktop login session. + </p> + + <p> + The "qemu:///system" family of URIs connect to a + libvirtd instance running as the privileged system account 'root'. + Thus the QEMU instances spawned from this driver may have much + higher privileges than the client application managing them. + The intended use case for this driver is server virtualization, + where the virtual machines may need to be connected to host + resources (block, PCI, USB, network devices) whose access requires + elevated privileges. + </p> + + <h3><a name="securitydac">POSIX DAC users/groups</a></h3> + + <p> + In the "session" instance, the POSIX DAC model restricts QEMU virtual + machines (and libvirtd in general) to only have access to resources + with the same user/group ID as the client application. There is no + finer level of configuration possible for the "session" instances. + </p> + + <p> + In the "system" instance, libvirt releases from 0.7.0 onwards allow + control over the user/group that the QEMU virtual machines are run + as. A build of libvirt with no configuration parameters set will + still run QEMU processes as root:root. It is possible to change + this default by using the --with-qemu-user=$USERNAME and + --with-qemu-group=$GROUPNAME arguments to 'configure' during + build. It is strongly recommended that vendors build with both + of these arguments set to 'qemu'. Regardless of this build time + default, administrators can set a per-host default setting in + the <code>/etc/libvirt/qemu.conf</code> configuration file via + the <code>user=$USERNAME</code> and <code>group=$GROUPNAME</code> + parameters. When a non-root user or group is configured, the + libvirt QEMU driver will change uid/gid to match immediately + before executing the QEMU binary for a virtual machine. + </p> + + <p> + If QEMU virtual machines from the "system" instance are being + run as non-root, there will be greater restrictions on what + host resources the QEMU process will be able to access. The + libvirtd daemon will attempt to manage permissions on resources + to minise the likelihood of unintentionale security denials, + but the administrator / application developer must be aware of + some of the consequences / restrictions. + </p> + + <ul> + <li> + <p> + The directories <code>/var/run/libvirt/qemu/</code>, + <code>/var/lib/libvirt/qemu/</code> and + <code>/var/cache/libvirt/qemu/</code> must all have their + ownership set to match the user / group ID that QEMU + guests will be run as. If the vendor has set a non-root + user/group for the QEMU driver at build time, the + permissions should be set automatically at install time. + If a host administrator customizes user/group in + <code>/etc/libvirt/qemu.conf</code>, they will need to + manually set the ownership on these directories. + </p> + </li> + <li> + <p> + When attaching PCI and USB devices to a QEMU guest, + QEMU will need to access files in <code>/dev/bus/usb</code> + and <code>/sys/bus/devices</code>. The libvirtd daemon + will automatically set the ownership on specific devices + that are assigned to a guest at start time. There should + not be any need for administrator changes in this respect. + </p> + </li> + <li> + <p> + Any files/devices used as guest disk images must be + accessible to the user/group ID that QEMU guests are + configured to run as. The libvirtd daemon will automatically + set the ownership of the file/device path to the correct + user/group ID. Applications / administrators must be aware + though that the parent directory permissions may still + deny access. The directories containing disk images + must either have their ownership set to match the user/group + configured for QEMU, or their UNIX file permissions must + have the 'execute/search' bit enabled for 'others'. + </p> + <p> + The simplest option is the latter one, of just enabling + the 'execute/search' bit. For any directory to be used + for storing disk images, this can be achived by running + the following command on the directory itself, and any + parent directories + </p> +<pre> + chmod o+x /path/to/directory +</pre> + <p> + In particular note that if using the "system" instance + and attempting to store disk images in a user home + directory, the default permissions on $HOME are typically + too restrictive to allow access. + </p> + </li> + </ul> + + <h3><a name="securitycap">Linux DAC capabilities</a></h3> + + <p> + The libvirt QEMU driver has a build time option allowing it to use + the <a href="http://people.redhat.com/sgrubb/libcap-ng/index.html">libcap-ng</a> + library to manage process capabilities. If this build option is + enabled, then the QEMU driver will use this to ensure that all + process capabilities are dropped before executing a QEMU virtual + machine. Process capabilities are what gives the 'root' account + its high power, in particular the CAP_DAC_OVERRIDE capability + is what allows a process running as 'root' to access files owned + by any user. + </p> + + <p> + If the QEMU driver is configured to run virtual machines as non-root, + then they will already loose all their process capabilities at time + of startup. The Linux capability feature is thus aimed primarily at + the scenario where the QEMU processes are running as root. In this + case, before launching a QEMU virtual machine, libvirtd will use + libcap-ng APIs to drop all process capabilities. It is important + for administrators to note that this implies the QEMU process will + <strong>only</strong> be able to access files owned by root, and + not files owned by any other user. + </p> + + <p> + Thus, if a vendor / distributor has configured their libvirt package + to run as 'qemu' by default, a number of changes will be required + before an administrator can change a host to run guests as root. + In particular it will be neccessary to change ownership on the + directories <code>/var/run/libvirt/qemu/</code>, + <code>/var/lib/libvirt/qemu/</code> and + <code>/var/cache/libvirt/qemu/</code> back to root, in addition + to changing the <code>/etc/libvirt/qemu.conf</code> settings. + </p> + + <h3><a name="securityselinux">SELinux MAC basic confinement</a></h3> + + <p> + The basic SELinux protection for QEMU virtual machines is intended to + protect the host OS from a compromised virtual machine process. There + is no protection between guests. + </p> + + <p> + In the basic model, all QEMU virtual machines run under the confined + domain <code>root:system_r:qemu_t</code>. It is required that any + disk image assigned to a QEMU virtual machine is labelled with + <code>system_u:object_r:virt_image_t</code>. In a default deployment, + package vendors/distributor will typically ensure that the directory + <code>/var/lib/libvirt/images</code> has this label, such that any + disk images created in this directory will automatically inherit the + correct labelling. If attempting to use disk images in another + location, the user/administrator must ensure the directory has be + given this requisite label. Likewise physical block devices must + be labelled <code>system_u:object_r:virt_image_t</code>. + </p> + <p> + Not all filesystems allow for labelling of individual files. In + particular NFS, VFat and NTFS have no support for labelling. In + these cases administrators must use the 'context' option when + mounting the filesystem to set the default label to + <code>system_u:object_r:virt_image_t</code>. In the case of + NFS, there is an alternative option, of enabling the <code>virt_use_nfs</code> + SELinux boolean. + </p> + + <h3><a name="securitysvirt">SELinux MAC sVirt confinement</a></h3> + + <p> + The SELinux sVirt protection for QEMU virtual machines builds to the + basic level of protection, to also allow individual guests to be + protected from each other. + </p> + + <p> + In the sVirt model, each QEMU virtual machine runs under its own + confined domain, which is based on <code>system_u:system_r:svirt_t:s0</code> + with a unique category appended, eg, <code>system_u:system_r:svirt_t:s0:c34,c44</code>. + The rules are setup such that a domain can only access files which are + labelled with the matching category level, eg + <code>system_u:object_r:svirt_image_t:s0:c34,c44</code>. This prevents one + QEMU process accessing any file resources that are prevent to another QEMU + process. + </p> + + <p> + There are two ways of assigning labels to virtual machines under sVirt. + In the default setup, if sVirt is enabled, guests will get an automatically + assigned unique label each time they are booted. The libvirtd daemon will + also automatically relabel exclusive access disk images to match this + label. Disks that are marked as <shared> will get a generic + label <code>system_u:system_r:svirt_image_t:s0</code> allowing all guests + read/write access them, while disks marked as <readonly> will + get a generic label <code>system_u:system_r:svirt_content_t:s0</code> + which allows all guests read-only access. + </p> + + <p> + With statically assigned labels, the application should include the + desired guest and file labels in the XML at time of creating the + guest with libvirt. In this scenario the application is responsible + for ensuring the disk images & similar resources are suitably + labelled to match, libvirtd will not attempt any relabelling. + </p> + + <p> + If the sVirt security model is active, then the node capabilties + XML will include its details. If a virtual machine is currently + protected by the security model, then the guest XML will include + its assigned labels. If enabled at compile time, the sVirt security + model will always be activated if SELinux is available on the host + OS. To disable sVirt, and revert to the basic level of SELinux + protection (host protection only), the <code>/etc/libvirt/qemu.conf</code> + file can be used to change the setting to <code>security_driver="none"</code> + </p> + + + <h3><a name="securityacl">Cgroups device ACLs</a></h3> + + <p> + Recent Linux kernels have a capability known as "cgroups" which is used + for resource management. It is implemented via a number of "controllers", + each controller covering a specific task/functional area. One of the + available controllers is the "devices" controller, which is able to + setup whitelists of block/character devices that a cgroup should be + allowed to access. If the "devices" controller is mounted on a host, + then libvirt will automatically create a dedicated cgroup for each + QEMU virtual machine and setup the device whitelist so that the QEMU + process can only access shared devices, and explicitly disks images + backed by block devices. + </p> + + <p> + The list of shared devices a guest is allowed access to is + </p> + + <pre> + /dev/null, /dev/full, /dev/zero, + /dev/random, /dev/urandom, + /dev/ptmx, /dev/kvm, /dev/kqemu, + /dev/rtc, /dev/hpet, /dev/net/tun + </pre> + + <p> + In the event of unanticipated needs arising, this can be customized + via the <code>/etc/libvirt/qemu.conf</code> file. + To mount the cgroups device controller, the following command + should be run as root, prior to starting libvirtd + </p> + + <pre> + mkdir /dev/cgroup + mount -t cgroup none /dev/cgroup -o devices + </pre> + + <p> + libvirt will then place each virtual machine in a cgroup at + <code>/dev/cgroup/libvirt/qemu/$VMNAME/</code> + </p> + <h2><a name="imex">Import and export of libvirt domain XML configs</a></h2> <p>The QEMU driver currently supports a single native -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

3 3

[libvirt] Add support for (qcow*) volume encryption (v3)
by Miloslav Trmač 20 Aug '09

20 Aug '09

Hello, the following patches add full support for qcow/qcow2 volume encryption, assuming a client that supports it. (Main changes since the previous version: * Significantly change the internal API, basing it on virSecretPtr instead of (virConnectPtr, secret_id); details in patch 1 * Make virsh commands more similar to the existing commands * Remove <encryption format='unencrypted'/> * Rename "secret_id" to "uuid" in the XML and API * Use "unsigned char *" for secret value See the specific patch change logs for more details; patches without change logs are unchanged.) New XML tags are defined to represent encryption parameters (currently format and passphrase, more can be added in the future), e.g. <encryption format='qcow'> <secret type='passphrase' uuid='724d95f2-0ed2-6ff9-84d0-0f3d1618428d'/> </encryption> The <encryption> tag can be added to a <volume> node passed to virStorageVolCreateXML() to create an encrypted volume, or to a <disk> node inside a <domain> to specify what encryption parameters to use for a domain. uuid above refers to a separately-managed secret, which was created using virSecretDefineXML() and set using virSecretSetValue(). Other properties of the secret can be managed using an XML representation. Detailed documentation of the formats and features is inside the patches.

4 33