- Devel - libvirt List Archives

[libvirt] Schedule for next release 0.7.1
by Daniel Veillard 27 Aug '09

27 Aug '09

Hello, following a bit too much backlog on libxml2 for the last years and an happy familly event, I have been a bit sidetracked in the last weeks and will be in the near future. I know there is a tons of pending patches, updates for drives and cleanups waiting for a release. I will try to make sure everything is reviewed and pushed by end of next week, which would allow for a 0.7.1 release by Sept 11, I hope this won't generate too much disruption while the initial schedule was at the end of the month ! thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

1 0

[libvirt] [PATCH] virsh vol-key segfault fix
by Pritesh Kothari 27 Aug '09

27 Aug '09

Hi, There is a bug in virsh command vol-key, it segfaults cause the fourth option to vshCommandOptVolBy() is null instead of "pool" on virsh.c:5209. The patch to fix this is attached. Regards, Pritesh

2 1

[libvirt] ANNOUNCE: perl bindings Sys-Virt release 0.2.1
by Daniel P. Berrange 25 Aug '09

25 Aug '09

FYI I have just uploaded a new release 0.2.1 of the libvirt perl bindings, Sys-Virt, to CPAN. http://search.cpan.org/dist/Sys-Virt/ Direct download link http://search.cpan.org/CPAN/authors/id/D/DA/DANBERR/Sys-Virt-0.2.1.tar.gz Changes since 0.2.0: - Add all new APIs upto libvirt 0.6.4 (network interfaces, storage volume cloning and node device create/delete) - Fix off-by-1 in UUID stringification - Set default values for optional params. - Use SvOK check for potentially undefined string params - Export all the virError related constants - Fix some memory leaks in C layer - Remove bogus module imports from test suite There will shortly be another release that adds support for 0.7.0 newly added APIs. Regards, Daniel -- |: 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 :|

1 0

[libvirt] new API to get list of *all* interfaces matching a MAC
by Laine Stump 25 Aug '09

25 Aug '09

(This probably seems like overanalysis of a simple problem. That's what I'm best at ;-) Due to oversight, the function virInterfaceLookupByMACString() can only return a single interface, but it's possible that a host may have more than one interface with the same MAC. Since the API had already been released by the time we realized this, the existing function will remain and a new one added that can return a list of interfaces. This new API will need to deal with the fact that the list length is effectively unbounded. I can see three ways of dealing with this, and want to learn which is preferred by others before spending time on the implementation. 1) The array containing the list is allocated by libvirt, freed by caller. int virInterfaceLookupAllByMACString(virConnectPtr conn, const char *mac, virInterfacePtr **matches); "matches" will point to an array of virInterfacePtr, and that array's length will be the return value. This array will be allocated by libvirt, but must be freed by the application when it's finished. Usage (ignoring errors, of course :-): virInterfacePtr *matches; int matchCt; matchCt = virInterfaceLookupAllByMACString(conn, "00:01:02:03:04:05", &matches); for (i = 0; i < matchCt; i++) { /* do something with an interface */ virInterfaceFree(matches[i]); } free(matches); 2) The array containing the list is allocated by the application, and its length sent to libvirt. Libvirt then returns the actual number of matches, which may be greater than the max length sent by the application, but only copies as many as the application asked for. If the application sends 0 as maxMatches and/or a NULL pointer instead of a pointer to an array, libvirt will return the actual length, but not the list itself. For example, if there are 3 interfaces matching, and maxMatches is set to 2, the function will still return 3, but only set two items in the matches array. int virInterfaceLookupAllByMACString(virConnectPtr conn, const char *mac, virInterfacePtr **matches, int maxMatches); This puts all the functionality needed in a single API function, but it will generally need to be called twice - once to learn the length, and the second (after allocating an array of appropriate size) to retrieve the list. virInterfacePtr *matches; int matchCtA, matchCtB; matchCtA = virInterfaceLookupAllByMACString(conn, "00:01:02:03:04:05", NULL, 0); matches = malloc (matchCt * sizeof(virInterfacePtr)); matchCtB = virInterfaceLookupAllByMACString(conn, "00:01:02:03:04:05", &matches, matchCt); for (i = 0; i < min(matchCtA, matchCtB); i++) { /* do something with an interface */ virInterfaceFree(matches[i]); } free(matches); 3) Again, the array containing the list is allocated by the application, but it learns the proper length to allocate by calling a different API function. The lookup function itself returns the number of interfaces delivered in the array, rather than the number of matches it *wanted* to put in the array. int virInterfaceCountAllByMACString(virConnectPtr conn, const char *mac); int virInterfaceLookupAllByMACString(virConnectPtr conn, const char *mac, virInterfacePtr **matches, int maxMatches); Usage: virInterfacePtr *matches; int matchCt; matchCt = virInterfaceCountAllByMACString(conn, "00:01:02:03:04:05"); matches = malloc (matchCt * sizeof(virInterfacePtr)); matchCt = virInterfaceLookupAllByMACString(conn, "00:01:02:03:04:05", &matches, matchCt); for (i = 0; i < matchCt; i++) { /* do something with an interface */ virInterfaceFree(matches[i]); } free(matches); ******* (3) is closest to behavior of other libvirt APIs, but requires more functions in the API, and could lead to situations where newly added interfaces aren't in the list (if a new interface is added between calling the Count function and the Lookup function) . (2) has a more compact API (one which matches the netcf API exactly), but has the same problems with potential bad counts. (1) seems like the cleanest API, but I don't know what others' opinions are on having libvirt allocate the array (since that's not how its done for other similar things, eg virConnectListInterface, virConnectListNetworks, etc), or maybe there's some limitation of the RPC I haven't considered that makes it unfeasible. Which should I implement?

3 5

[libvirt] error: failed to connect to hypervisor (openvz)
by AL.Gabteni 25 Aug '09

25 Aug '09

Hi im trying to use libvirt with openvz since 2 days but without luck ,i allways get error: failed to connect to hypervisor (openvz) ; the test connection is working , any help will be appreciated . Regards

2 1

[libvirt] Small patch for ruby-libvirt to add api wrapping for DomainAttachDevice and DomainDetachDevice
by Jacob Leaver 25 Aug '09

25 Aug '09

--- ext/libvirt/_libvirt.c.orig 2009-01-19 18:24:22.000000000 -0800 +++ ext/libvirt/_libvirt.c 2009-01-19 18:24:39.000000000 -0800 @@ -728,6 +728,21 @@ gen_call_void(virDomainSave, conn(s), domain_get(s), StringValueCStr(to)); } +/* + * Call +virDomainAttachDevice+[http://www.libvirt.org/html/libvirt-libvirt.html#virDomainAttachDevice] + */ +VALUE libvirt_dom_attach_device_xml(VALUE s, VALUE xml) { + virDomainPtr dom = domain_get(s); + return virDomainAttachDevice(dom, StringValueCStr(xml)); +} + +/* + * Call +virDomainDetachDevice+[http://www.libvirt.org/html/libvirt-libvirt.html#virDomainDetachDevice] + */ +VALUE libvirt_dom_detach_device_xml(VALUE s, VALUE xml) { + virDomainPtr dom = domain_get(s); + return virDomainDetachDevice(dom, StringValueCStr(xml)); +} /* * Call +virDomainCoreDump+[http://www.libvirt.org/html/libvirt-libvirt.html#virDomainCoreDump] @@ -1921,7 +1936,8 @@ rb_define_method(c_domain, "autostart", libvirt_dom_autostart, 0); rb_define_method(c_domain, "autostart=", libvirt_dom_autostart_set, 1); rb_define_method(c_domain, "free", libvirt_dom_free, 0); - + rb_define_method(c_domain, "attach_device", libvirt_dom_attach_device_xml, 1); + rb_define_method(c_domain, "detach_device", libvirt_dom_detach_device_xml, 1); /* * Class Libvirt::Domain::Info */

2 1

[libvirt] OS Eject & Removable Media
by Jacob Leaver 25 Aug '09

25 Aug '09

Greetings all, First, I'd like to thank everyone that's working on the libvirt project, it's certainly making my life a better place. Second, (there's always a second, eh?) I am using libvirtd 0.6.4 and KVM qemu-kvm-0.10.5, and when a guest OS ejects media, libvirtd doesn't seem to pick up the change. Looking through the API, I don't see a provision to probe for a list of block devices, so although qemu and kvm know about media changes, I don't think there's a way for libvirtd to know about them. I've scanned the list archives, and haven't really seen any discussion of this, but I'd be more than happy to be directed somewhere. Also, I have a couple of minor and hackish patches for the ruby bindings for attaching and removing devices, if someone's interested. Sincerely, Jacob Leaver

2 2

[libvirt] network not autostarting
by Mariano Absatz 24 Aug '09

24 Aug '09

Hi, I'm using libvirt-bin 0.6.1 from the standard ubuntu repositories under ubuntu 9.04 (jaunty). I have, beside the default network, another one called "isolated" using the following description file: <network> <name>isolated</name> <uuid>51cffbcc-88f5-4edc-a81c-1765c1045691</uuid> <bridge name='virbr%d' stp='on' forwardDelay='0' /> <ip address='10.3.14.1' netmask='255.255.255.0'> <dhcp> <range start='10.3.14.128' end='10.3.14.254' /> </dhcp> </ip> </network> I have configured it with "net-autostart" and the symlink in the /etc/libvirt/qemu/networks/autostart/ autostart directory is just fine... however, it isn't starting automatically anymore (and it used to start up before)... Any hints on what to look up and where? TIA. -- Mariano Absatz - "El Baby" el.baby(a)gmail.com www.clueless.com.ar -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- _ | / \ | \ / ASCII RIBBON CAMPAIGN | X AGAINST HTML MAIL | / \ www.asciiribbon.org | | | -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- * TagZilla 0.066 * http://tagzilla.mozdev.org

2 1

[libvirt] Communicating with libvirt from windows
by anuj rampal 24 Aug '09

24 Aug '09

Hi, I have KVM running on one of my machine and I can controle the geues OS using the virt-manager and Virsh tools. Now what I want to do is talk to libvirt from my windows machine. I tried to compile libvirt for windows but couldnt get any success with that. Is there a way by which i can talk to libvirt without this client..?? As in can i talk to libvirt over TCP sockets using xml (something like that)...?? Thanks a lot in advance. Regards Anuj

4 3

[libvirt] [PATCH] Ignores EOPNOTSUPP when attempting to access an image on an NFS share.
by Darryl L. Pierce 21 Aug '09

21 Aug '09

rhbz#517157 Signed-off-by: Darryl L. Pierce <dpierce(a)redhat.com> --- src/security_selinux.c | 23 ++++++++++++++++------- 1 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/security_selinux.c b/src/security_selinux.c index 0db9f49..f1e3f17 100644 --- a/src/security_selinux.c +++ b/src/security_selinux.c @@ -323,6 +323,8 @@ SELinuxSetFilecon(virConnectPtr conn, const char *path, char *tcon) VIR_INFO("Setting SELinux context on '%s' to '%s'", path, tcon); if (setfilecon(path, tcon) < 0) { + int setfilecon_errno = errno; + if (getfilecon(path, &econ) >= 0) { if (STREQ(tcon, econ)) { freecon(econ); @@ -331,14 +333,21 @@ SELinuxSetFilecon(virConnectPtr conn, const char *path, char *tcon) } freecon(econ); } - virSecurityReportError(conn, VIR_ERR_ERROR, - _("%s: unable to set security context " - "'\%s\' on %s: %s."), __func__, - tcon, - path, - virStrerror(errno, ebuf, sizeof ebuf)); - if (security_getenforce() == 1) + + /* if the error complaint is related to an image hosted on + * an nfs mount, then ignore it. + * rhbz 517157 + */ + if (setfilecon_errno != EOPNOTSUPP) { + virSecurityReportError(conn, VIR_ERR_ERROR, + _("%s: unable to set security context " + "'\%s\' on %s: %s."), __func__, + tcon, + path, + virStrerror(errno, ebuf, sizeof ebuf)); + if (security_getenforce() == 1) return -1; + } } return 0; } -- 1.6.2.5

3 2

[libvirt] [PATCH] spec file: add URL to Source tag
by Mark McLoughlin 21 Aug '09

21 Aug '09

See https://fedoraproject.org/wiki/Packaging:SourceURL --- libvirt.spec.in | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index dad97ec..00c394c 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -81,7 +81,7 @@ Version: @VERSION@ Release: 1%{?dist}%{?extra_release} License: LGPLv2+ Group: Development/Libraries -Source: libvirt-%{version}.tar.gz +Source: http://libvirt.org/sources/libvirt-%{version}.tar.gz BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root URL: http://libvirt.org/ BuildRequires: python-devel -- 1.6.2.5

1 0

[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 20 Aug '09

20 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 19 Aug '09

19 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 19 Aug '09

19 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č 19 Aug '09

19 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

Re: [libvirt] [PATCH 07/20] Secret manipulation step 7: Local driver
by Miloslav Trmac 19 Aug '09

19 Aug '09

----- "Daniel Veillard" <veillard(a)redhat.com> wrote: > On Sun, Aug 16, 2009 at 10:48:00PM +0200, Miloslav Trmač wrote: > > This implementation stores the secrets in an unencrypted text file, > > for simplicity in implementation and debugging. > > > > (Symmetric encryption, e.g. using gpgme, will not be difficult to add. > > Because the TLS private key used by libvirtd is stored unencrypted, > > encrypting the secrets file does not currently provide much additional > > security.) > > What if we change our mind in some time, would there be any obstacle > to dynamically detecting things are not encrypted and switching to > a crypted file transparently? Two simple ways: 1) use a different file name, e,g. secrets.gpg 2) check the start of the file, currently the file always starts with "id "; I can add a proper magic number if necessary. > > +static int > > +writeBase64Data(virConnectPtr conn, int fd, const char *field, > > + const void *data, size_t size) > > +{ > > + int ret = -1; > > + char *base64 = NULL; > > + > > + if (writeString(conn, fd, field) < 0 || writeString(conn, fd, " > ") < 0) > > + goto cleanup; > > + > > + base64_encode_alloc(data, size, &base64); > > where is base64_encode_alloc coming from ? gnulib - in a later patch I'm afraid, I'll reorder it. > > +static int > > +saveSecrets(virConnectPtr conn, virSecretDriverStatePtr driver) > > +{ > > + const virSecretEntry *secret; > > + char *tmp_path = NULL; > > + int fd = -1, ret = -1; > > + > > + if (virAsprintf(&tmp_path, "%sXXXXXX", driver->filename) < 0) > { > > + virReportOOMError(conn); > > + goto cleanup; > > + } > > + fd = mkstemp (tmp_path); > > Hum even if unencrypted, we should make sure of the mode of the file > beforehand... Good point. > isn't there a safer equivalent (possibly made postable by gnulib ?) Grepping of gnulib does not reveal any. > > + if (fd == -1) { > > + virReportSystemError (conn, errno, _("mkstemp(\"%s\") failed"), > > + tmp_path); > > + goto cleanup; > > + } > > + > > + for (secret = driver->secrets; secret != NULL; > > + secret = secret->next) { > > + if (!secret->ephemeral && writeSecret(conn, fd, secret) < 0) > > + goto cleanup; > > + } > > + close(fd); > > + fd = -1; > > + if (rename(tmp_path, driver->filename) < 0) { > > + virReportSystemError (conn, errno, _("rename(%s, %s) failed"), tmp_path, > > + driver->filename); > > + goto cleanup; > > + } > > Hum, the whole set smells fishy, we are creating a temp file, without > mode checked, then moving that file somewhere else. I would ratehr have > internal APIsdeling with a dump to memory and then a single open, safe > and directly to the driver->filename. "Somewhere else" is in the same directory. The mkstemp() + rename() is used to make sure the secrets file is replaced atomically, without losing any data if a save is interrupted in the middle. > > +static int > > +parseBase64String(virConnectPtr conn, const char *base64, char > **string) > > +{ > > + char *tmp; > > + size_t size; > > + > > + if (!base64_decode_alloc(base64, strlen(base64), &tmp, &size)) > { > > where is base64_decode_alloc coming from ? gnulib > > + if (stat(driver->filename, &st) < 0) { > > + if (errno == ENOENT) > > + return 0; > > + virReportSystemError (conn, errno, _("cannot stat '%s'"), > > + driver->filename); > > + goto cleanup; > > + } > > + if ((size_t)st.st_size != st.st_size || (size_t)(st.st_size + 1) == 0) { > > + virSecretReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", > > + _("secrets file does not fit in memory")); > > + goto cleanup; > > + } > > + > > + fd = open(driver->filename, O_RDONLY); > > + if (fd == -1) { > > + virReportSystemError (conn, errno, _("cannot open '%s'"), > > + driver->filename); > > + goto cleanup; > > + } > > stat()/open() introduces a small race, I think open() and > then fdstat() is a bit cleaner, not a big deal though Technically there is still a race with fstat() - but fstat is a bit better. > > + if (VIR_ALLOC_N(contents, st.st_size + 1) < 0) { > > + virReportOOMError(conn); > > + goto cleanup; > > + } > > + if (saferead(fd, contents, st.st_size) != st.st_size) { > > + virReportSystemError (conn, errno, _("cannot read '%s'"), > > + driver->filename); > > + goto cleanup; > > + } > > + close(fd); > > + fd = -1; > > contents[st.st_size] = 0; > needed here. VIR_ALLOC_N automatically zeroes the memory. I'll fix all other issues you have mentioned. Thanks for the review. Mirek

1 0

Re: [libvirt] [PATCH 05/20] Secret manipulation step 5: RPC client
by Miloslav Trmac 19 Aug '09

19 Aug '09

----- "Daniel Veillard" <veillard(a)redhat.com> wrote: > On Sun, Aug 16, 2009 at 10:47:58PM +0200, Miloslav Trmač wrote: > > +static virDrvOpenStatus > > +remoteSecretOpen (virConnectPtr conn, > > + virConnectAuthPtr auth, > > + int flags) > > +{ > > + if (inside_daemon) > > + return VIR_DRV_OPEN_DECLINED; > > + > > + if (conn && > > + conn->driver && > > + STREQ (conn->driver->name, "remote")) { > > + struct private_data *priv; > > + > > + /* If we're here, the remote driver is already > > + * in use due to a) a QEMU uri, or b) a remote > > + * URI. So we can re-use existing connection > > + */ > > Hum, is that comment right ? We are certainly using a remote URI in > any case there, isn't it ? The same comment is in in all of the other "subdriver" remote...Open () routines. It's not quite clear what "remote" means where, as far as I can see "remote" means an "explicitly remote" URL, e.g. anything+tls://hostname/..., and "QEMU" means "qemu://hostname/..." (rejected by qemu_driver). Mirek

2 1

Re: [libvirt] [PATCH 01/20] Secret manipulation step 1: Public API
by Miloslav Trmac 19 Aug '09

19 Aug '09

----- "Daniel Veillard" <veillard(a)redhat.com> wrote: > On Sun, Aug 16, 2009 at 10:47:54PM +0200, Miloslav Trmač wrote: > > Rather than add explicit accessors for attributes of secrets, and > > hard-code the "secrets are related to storage volumes" association > in > > the API, the API uses XML to manipulate the association as well as > > other attributes, similarly to other areas of libvirt. > > > > The user can set attributes of the secret using XML, e.g. > > <secret ephemeral='no' private='yes'> > > <uuid>b8eecf55-798e-4db7-b2dd-025b0cf08a36</uuid> > > <volume>/var/lib/libvirt/images/mail.img</volume> > > <description>LUKS passphrase for our mail server</description> > > </secret> > > If <uuid/> is not specified, it is chosen automatically. > > Should secret always be tied to volumes. The API is generic enough > that we should make sure we can use this later to get priviledged > access to other resources, though right now I don't have a good > example in mind. The secret is not "tied" to a volume - the volume is an optional attribute. A libvirt user can ignore the volume field completely, and manage the secrets separately - but other clients, e.g. virt-manager, can use the volume attribute to locate a secret for a given volume without storing any data outside of libvirt. > > - s/secret_id/uuid/g > > Not sure I catch that, An earlier patch was calling the ID of a secret object "secret_id" everywhere, leading to constructs like <secret secret_id=.../> In an earlier patch review I was asked to change that. > > - use "unsigned char *" for secret value > > fine, assuming 0 terninated. though this may not work with everything > but since we need to import/export to XML, unless uuencoding the value > we won't be able to embed something which is not part of XML character > range (#x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] | > [#x10000-#x10FFFF]) unicode code points. No, the value is an arbitrary set of bytes that is never passed through XML. Originally it was (void *value, size_t value_size), the change to (unsigned char *value, size_t value_size) was requested in an earlier review. > > diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in > > new file mode 100644 > > index 0000000..7471bf7 > > --- /dev/null > > +++ b/docs/formatsecret.html.in > > @@ -0,0 +1,52 @@ > > +<html> > > + <body> > > + <h1>Secret XML format</h1> > > + > > + <ul id="toc"></ul> > > it's good to have a document from the start :-) but this lacks a bit > of the intended use for the API, i.e. where this may be required to > use it. Those parts are documented in later patches that add references to secrets to the <volume> and <domain> XML formats. > > +virSecretPtr virSecretDefineXML (virConnectPtr conn, > > + const char *xml); > > Let's add an "unsigned int flags" to virSecretDefineXML() especially > as we don't know yet the scope of future usages. > > > +char * virSecretGetXMLDesc (virSecretPtr secret); > > And also add an "unsigned int flags" to virSecretGetXMLDesc > Will do. > > +int virSecretSetValue (virSecretPtr secret, > > + const unsigned char *value, > > + size_t value_size); > > +unsigned char * virSecretGetValue (virSecretPtr secret, > > + size_t *value_size); > > Ah, so we aren't relying on 0 termination, but in that case the value > need to be uuencoded in the XML, and that should be made clear in the > API description. Actually not having the secret completely in the clear > in the XML dump sounds like a good idea. The values are never transferred in the XML, only using the virSecret[GS]etValue functions. (Storing secrets in XML was a major objection against the first version of my patch e.g. because XML tends to be revealed in various debugging dumps.) Mirek

3 3

[libvirt] OpenVZ : The restriction of domain name should be addressed
by Yuji NISHIDA 19 Aug '09

19 Aug '09

Hi, "The current libvirt OpenVZ driver has a restriction that the domain names must match the OpenVZ container VEID" I really want to have domain name as character, not integer. It could help me very much if any patch against this problem would be released. Is there anyone tackling this problem now?( or going to? ) Otherwise, any hint would help me. Regards. ----- Yuji Nishida nishidy(a)nict.go.jp

5 5