The code is splattered with a mix of
sizeof foo
sizeof (foo)
sizeof(foo)
Standardize on sizeof(foo) and add a syntax check rule to
enforce it
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Return statements with parameter enclosed in parentheses were modified
and parentheses were removed. The whole change was scripted, here is how:
List of files was obtained using this command:
git grep -l -e '\<return\s*([^()]*\(([^()]*)[^()]*\)*)\s*;' | \
grep -e '\.[ch]$' -e '\.py$'
Found files were modified with this command:
sed -i -e \
's_^\(.*\<return\)\s*(\(\([^()]*([^()]*)[^()]*\)*\))\s*\(;.*$\)_\1 \2\4_' \
-e 's_^\(.*\<return\)\s*(\([^()]*\))\s*\(;.*$\)_\1 \2\3_'
Then checked for nonsense.
The whole command looks like this:
git grep -l -e '\<return\s*([^()]*\(([^()]*)[^()]*\)*)\s*;' | \
grep -e '\.[ch]$' -e '\.py$' | xargs sed -i -e \
's_^\(.*\<return\)\s*(\(\([^()]*([^()]*)[^()]*\)*\))\s*\(;.*$\)_\1 \2\4_' \
-e 's_^\(.*\<return\)\s*(\([^()]*\))\s*\(;.*$\)_\1 \2\3_'
No thanks to 64-bit windows, with 64-bit pid_t, we have to avoid
constructs like 'int pid'. Our API in libvirt-qemu cannot be
changed without breaking ABI; but then again, libvirt-qemu can
only be used on systems that support UNIX sockets, which rules
out Windows (even if qemu could be compiled there) - so for all
points on the call chain that interact with this API decision,
we require a different variable name to make it clear that we
audited the use for safety.
Adding a syntax-check rule only solves half the battle; anywhere
that uses printf on a pid_t still needs to be converted, but that
will be a separate patch.
* cfg.mk (sc_correct_id_types): New syntax check.
* src/libvirt-qemu.c (virDomainQemuAttach): Document why we didn't
use pid_t for pid, and validate for overflow.
* include/libvirt/libvirt-qemu.h (virDomainQemuAttach): Tweak name
for syntax check.
* src/vmware/vmware_conf.c (vmwareExtractPid): Likewise.
* src/driver.h (virDrvDomainQemuAttach): Likewise.
* tools/virsh.c (cmdQemuAttach): Likewise.
* src/remote/qemu_protocol.x (qemu_domain_attach_args): Likewise.
* src/qemu_protocol-structs (qemu_domain_attach_args): Likewise.
* src/util/cgroup.c (virCgroupPidCode, virCgroupKillInternal):
Likewise.
* src/qemu/qemu_command.c(qemuParseProcFileStrings): Likewise.
(qemuParseCommandLinePid): Use pid_t for pid.
* daemon/libvirtd.c (daemonForkIntoBackground): Likewise.
* src/conf/domain_conf.h (_virDomainObj): Likewise.
* src/probes.d (rpc_socket_new): Likewise.
* src/qemu/qemu_command.h (qemuParseCommandLinePid): Likewise.
* src/qemu/qemu_driver.c (qemudGetProcessInfo, qemuDomainAttach):
Likewise.
* src/qemu/qemu_process.c (qemuProcessAttach): Likewise.
* src/qemu/qemu_process.h (qemuProcessAttach): Likewise.
* src/uml/uml_driver.c (umlGetProcessInfo): Likewise.
* src/util/virnetdev.h (virNetDevSetNamespace): Likewise.
* src/util/virnetdev.c (virNetDevSetNamespace): Likewise.
* tests/testutils.c (virtTestCaptureProgramOutput): Likewise.
* src/conf/storage_conf.h (_virStoragePerms): Use mode_t, uid_t,
and gid_t rather than int.
* src/security/security_dac.c (virSecurityDACSetOwnership): Likewise.
* src/conf/storage_conf.c (virStorageDefParsePerms): Avoid
compiler warning.
In case the caller specifies that confined guests are required but the
security driver turns out to be 'none', we should return an error since
this driver clearly cannot meet that requirement. As a result of this
error, libvirtd fails to start when the host admin explicitly sets
confined guests are required but there is no security driver available.
Since security driver 'none' cannot create confined guests, we override
default confined setting so that hypervisor drivers do not thing they
should create confined guests.
This eliminates the warning message reported in:
https://bugzilla.redhat.com/show_bug.cgi?id=624447
It was caused by a failure to open an image file that is not
accessible by root (the uid libvirtd is running as) because it's on a
root-squash NFS share, owned by a different user, with permissions of
660 (or maybe 600).
The solution is to use virFileOpenAs() rather than open(). The
codepath that generates the error is during qemuSetupDiskCGroup(), but
the actual open() is in a lower-level generic function called from
many places (virDomainDiskDefForeachPath), so some other pieces of the
code were touched just to add dummy (or possibly useful) uid and gid
arguments.
Eliminating this warning message has the nice side effect that the
requested operation may even succeed (which in this case isn't
necessary, but shouldn't hurt anything either).
Curently security labels can be of type 'dynamic' or 'static'.
If no security label is given, then 'dynamic' is assumed. The
current code takes advantage of this default, and avoids even
saving <seclabel> elements with type='dynamic' to disk. This
means if you temporarily change security driver, the guests
can all still start.
With the introduction of sVirt to LXC though, there needs to be
a new default of 'none' to allow unconfined LXC containers.
This patch introduces two new security label types
- default: the host configuration decides whether to run the
guest with type 'none' or 'dynamic' at guest start
- none: the guest will run unconfined by security policy
The 'none' label type will obviously be undesirable for some
deployments, so a new qemu.conf option allows a host admin to
mandate confined guests. It is also possible to turn off default
confinement
security_default_confined = 1|0 (default == 1)
security_require_confined = 1|0 (default == 0)
* src/conf/domain_conf.c, src/conf/domain_conf.h: Add new
seclabel types
* src/security/security_manager.c, src/security/security_manager.h:
Set default sec label types
* src/security/security_selinux.c: Handle 'none' seclabel type
* src/qemu/qemu.conf, src/qemu/qemu_conf.c, src/qemu/qemu_conf.h,
src/qemu/libvirtd_qemu.aug: New security config options
* src/qemu/qemu_driver.c: Tell security driver about default
config
The old virRandom() API was not generating good random numbers.
Replace it with a new API virRandomBits which instead of being
told the upper limit, gets told the number of bits of randomness
required.
* src/util/virrandom.c, src/util/virrandom.h: Add virRandomBits,
and move virRandomInitialize
* src/util/util.h, src/util/util.c: Delete virRandom and
virRandomInitialize
* src/libvirt.c, src/security/security_selinux.c,
src/test/test_driver.c, src/util/iohelper.c: Update for
changes from virRandom to virRandomBits
* src/storage/storage_backend_iscsi.c: Remove bogus call
to virRandomInitialize & convert to virRandomBits
When sVirt is integrated with the LXC driver, it will be neccessary
to invoke the security driver APIs using only a virDomainDefPtr
since the lxc_container.c code has no virDomainObjPtr available.
Aside from two functions which want obj->pid, every bit of the
security driver code only touches obj->def. So we don't need to
pass a virDomainObjPtr into the security drivers, a virDomainDefPtr
is sufficient. Two functions also gain a 'pid_t pid' argument.
* src/qemu/qemu_driver.c, src/qemu/qemu_hotplug.c,
src/qemu/qemu_migration.c, src/qemu/qemu_process.c,
src/security/security_apparmor.c,
src/security/security_dac.c,
src/security/security_driver.h,
src/security/security_manager.c,
src/security/security_manager.h,
src/security/security_nop.c,
src/security/security_selinux.c,
src/security/security_stack.c: Change all security APIs to use a
virDomainDefPtr instead of virDomainObjPtr
This wires up the XML changes in the previous patch to let SELinux
labeling honor user overrides, as well as affecting the live XML
configuration in one case where the user didn't specify anything
in the offline XML.
I noticed that the logs contained messages like this:
2011-12-05 23:32:40.382+0000: 26569: warning : SELinuxRestoreSecurityFileLabel:533 : cannot lookup default selinux label for /nfs/libvirt/images/dom.img
for all my domain images living on NFS. But if we would just remember
that on domain creation that we were unable to set a SELinux label (due to
NFSv3 lacking labels, or NFSv4 not being configured to expose attributes),
then we could avoid wasting the time trying to clear the label on
domain shutdown. This in turn is one less point of NFS failure,
especially since there have been documented cases of virDomainDestroy
hanging during an attempted operation on a failed NFS connection.
* src/security/security_selinux.c (SELinuxSetFilecon): Move guts...
(SELinuxSetFileconHelper): ...to new function.
(SELinuxSetFileconOptional): New function.
(SELinuxSetSecurityFileLabel): Honor override label, and remember
if labeling failed.
(SELinuxRestoreSecurityImageLabelInt): Skip relabeling based on
override.
Network disks don't have paths to be resolved or files to be checked
for ownership. ee3efc41e6 checked this
for some image label functions, but was partially reverted in a
refactor. This finishes adding the check to each security driver's
set and restore label methods for images.
Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>
The pathname for the pipe for tunnelled migration is unresolvable. The
libvirt apparmor driver therefore refuses access, causing migration to
fail. If we can't resolve the path, the worst that can happen is that
we should have given permission to the file but didn't. Otherwise
(especially since this is a /proc/$$/fd/N file) the file is already open
and libvirt won't be refused access by apparmor anyway.
Also adjust virt-aa-helper to allow access to the
*.tunnelmigrate.dest.name files.
For more information, see https://launchpad.net/bugs/869553.
Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com>
Every instance of virCapsPtr must have the defaultConsoleTargetType
field set.
* src/security/virt-aa-helper.c: Add defaultConsoleTargetType to
virCapsPtr
While Xen only has a single paravirt console, UML, and
QEMU both support multiple paravirt consoles. The LXC
driver can also be trivially made to support multiple
consoles. This patch extends the XML to allow multiple
<console> elements in the XML. It also makes the UML
and QEMU drivers support this config.
* src/conf/domain_conf.c, src/conf/domain_conf.h: Allow
multiple <console> devices
* src/lxc/lxc_driver.c, src/xen/xen_driver.c,
src/xenxs/xen_sxpr.c, src/xenxs/xen_xm.c: Update for
internal API changes
* src/security/security_selinux.c, src/security/virt-aa-helper.c:
Only label consoles that aren't a copy of the serial device
* src/qemu/qemu_command.c, src/qemu/qemu_driver.c,
src/qemu/qemu_process.c, src/uml/uml_conf.c,
src/uml/uml_driver.c: Support multiple console devices
* tests/qemuxml2xmltest.c, tests/qemuxml2argvtest.c: Extra
tests for multiple virtio consoles. Set QEMU_CAPS_CHARDEV
for all console /channel tests
* tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-auto.args,
tests/qemuxml2argvdata/qemuxml2argv-channel-virtio.args
tests/qemuxml2argvdata/qemuxml2argv-console-virtio.args: Update
for correct chardev syntax
* tests/qemuxml2argvdata/qemuxml2argv-console-virtio-many.args,
tests/qemuxml2argvdata/qemuxml2argv-console-virtio-many.xml: New
test file
This patch fixes the regression with using named pipes for qemu serial
devices noted in:
https://bugzilla.redhat.com/show_bug.cgi?id=740478
The problem was that, while new code in libvirt looks for a single
bidirectional fifo of the name given in the config, then relabels that
and continues without looking for / relabelling the two unidirectional
fifos named ${name}.in and ${name}.out, qemu looks in the opposite
order. So if the user had naively created all three fifos, libvirt
would relabel the bidirectional fifo to allow qemu access, but qemu
would attempt to use the two unidirectional fifos and fail (because it
didn't have proper permissions/rights).
This patch changes the order that libvirt looks for the fifos to match
what qemu does - first it looks for the dual fifos, then it looks for
the single bidirectional fifo. If it finds the dual unidirectional
fifos first, it labels/chowns them and ignores any possible
bidirectional fifo.
(Note commit d37c6a3a (which first appeared in libvirt-0.9.2) added
the code that checked for a bidirectional fifo. Prior to that commit,
bidirectional fifos for serial devices didn't work because libvirt
always required the ${name}.(in|out) fifos to exist, and qemu would
always prefer those.
The AppArmor security driver adds only the path specified in the domain
XML for character devices of type 'pipe'. It should be using <path>.in
and <path>.out. We do this by creating a new vah_add_file_chardev() and
use it for char devices instead of vah_add_file(). Also adjust
valid_path() to accept S_FIFO (since qemu chardevs of type 'pipe' use
fifos). This is https://launchpad.net/bugs/832507
Commit 498d783 cleans up some of virtual file names for parsing strings
in memory. This patch cleans up (hopefuly) the rest forgotten by the
first patch.
This patch also changes all of the previously modified "filenames" to
valid URI's replacing spaces for underscores.
Changes to v1:
- Replace all spaces for underscores, so that the strings form valid
URI's
- Replace spaces in places changed by commit 498d783
While parsing XML strings from memory, the previous convention in
libvirt was to set the virtual file name to "domain.xml" or something
similar. This could potentialy trick the user into looking for a file
named domain.xml on the disk in an attempt to fix the error.
This patch changes these filenames to something that can't be as easily
confused for a valid filename.
Examples of error messages:
---------------------------
Error while loading file from disk:
15:07:59.015: 527: error : catchXMLError:709 : /path/to/domain.xml:1: StartTag: invalid element name
<domain type='kvm'><
--------------------^
Error while parsing definition in memory:
15:08:43.581: 525: error : catchXMLError:709 : (domain definition):2: error parsing attribute name
<name>vm1</name>
--^
The virSecurityManagerSetProcessFDLabel method was introduced
after a mis-understanding from a conversation about SELinux
socket labelling. The virSecurityManagerSetSocketLabel method
should have been used for all such scenarios.
* src/security/security_apparmor.c, src/security/security_apparmor.c,
src/security/security_driver.h, src/security/security_manager.c,
src/security/security_manager.h, src/security/security_selinux.c,
src/security/security_stack.c: Remove SetProcessFDLabel driver
This API labels all sockets created until ClearSocketLabel is called in
a way that a vm can access them (i.e., they are labeled with svirt_t
based label in SELinux).
The APIs are designed to label a socket in a way that the libvirt daemon
itself is able to access it (i.e., in SELinux the label is virtd_t based
as opposed to svirt_* we use for labeling resources that need to be
accessed by a vm). The new name reflects this.
In the Ubuntu development release we recently got a new udev that
moves /var/run to /run, /var/lock to /run/lock and /dev/shm to /run/shm.
This change in udev requires updating the apparmor security driver in
libvirt[1].
Attached is a patch that:
* adjusts src/security/virt-aa-helper.c to allow both
LOCALSTATEDIR/run/libvirt/**/%s.pid and /run/libvirt/**/%s.pid. While
the profile is not as precise, LOCALSTATEDIR/run/ is typically a symlink
to /run/ anyway, so there is no additional access (remember that
apparmor resolves symlinks, which is why this is still required even
if /var/run points to /run).
* adjusts example/apparmor/libvirt-qemu paths for /dev/shm
[1]https://launchpad.net/bugs/810270
--
Jamie Strandboge | http://www.canonical.com
The drivers were accepting domain configs without checking if those
were actually meant for them. For example the LXC driver happily
accepts configs with type QEMU.
Add a check for the expected domain types to the virDomainDefParse*
functions.
When no <seclabel> is present in the XML, the virDomainSeclabelDef
struct is left as all zeros. Unfortunately, this means it gets setup
as type=dynamic, with relabel=no, which is an illegal combination.
Change the 'bool relabel' attribute in virDomainSeclabelDef to
the inverse 'bool norelabel' so that the default initialization
is sensible
* src/conf/domain_conf.c, src/conf/domain_conf.h,
src/security/security_apparmor.c, src/security/security_selinux.c:
Replace 'relabel' with 'norelabel'
Add a new attribute to the <seclabel> XML to allow resource
relabelling to be enabled with static label usage.
<seclabel model='selinux' type='static' relabel='yes'>
<label>system_u:system_r:svirt_t:s0:c392,c662</label>
</seclabel>
* docs/schemas/domain.rng: Add relabel attribute
* src/conf/domain_conf.c, src/conf/domain_conf.h: Parse
the 'relabel' attribute
* src/qemu/qemu_process.c: Unconditionally clear out the
'imagelabel' attribute
* src/security/security_apparmor.c: Skip based on 'relabel'
attribute instead of label type
* src/security/security_selinux.c: Skip based on 'relabel'
attribute instead of label type and fill in <imagelabel>
attribute if relabel is enabled.
Normally the dynamic labelling mode will always use a base
label of 'svirt_t' for VMs. Introduce a <baselabel> field
in the <seclabel> XML to allow this base label to be changed
eg
<seclabel type='dynamic' model='selinux'>
<baselabel>system_u:object_r:virt_t:s0</baselabel>
</seclabel>
* docs/schemas/domain.rng: Add <baselabel>
* src/conf/domain_conf.c, src/conf/domain_conf.h: Parsing
of base label
* src/qemu/qemu_process.c: Don't reset 'model' attribute if
a base label is specified
* src/security/security_apparmor.c: Refuse to support base label
* src/security/security_selinux.c: Use 'baselabel' when generating
label, if available
Add a new security driver method for labelling an FD with
the process label, rather than the image label
* src/libvirt_private.syms, src/security/security_apparmor.c,
src/security/security_dac.c, src/security/security_driver.h,
src/security/security_manager.c, src/security/security_manager.h,
src/security/security_selinux.c, src/security/security_stack.c:
Add virSecurityManagerSetProcessFDLabel & impl
The virSecurityManagerSetFDLabel method is used to label
file descriptors associated with disk images. There will
shortly be a need to label other file descriptors in a
different way. So the current name is ambiguous. Rename
the method to virSecurityManagerSetImageFDLabel to clarify
its purpose
* src/libvirt_private.syms,
src/qemu/qemu_migration.c, src/qemu/qemu_process.c,
src/security/security_apparmor.c, src/security/security_dac.c,
src/security/security_driver.h, src/security/security_manager.c,
src/security/security_manager.h, src/security/security_selinux.c,
src/security/security_stack.c: s/FDLabel/ImageFDLabel/
Commit 12317957ec introduced an incompatible
architectural change for the AppArmor security driver. Specifically,
virSecurityManagerSetAllLabel() is now called much later in
src/qemu/qemu_process.c:qemuProcessStart(). Previously, SetAllLabel() was
called immediately after GenLabel() such that after the dynamic label (profile
name) was generated, SetAllLabel() would be called to create and load the
AppArmor profile into the kernel before qemuProcessHook() was executed. With
12317957ec, qemuProcessHook() is now called
before SetAllLabel(), such that aa_change_profile() ends up being called
before the AppArmor profile is loaded into the kernel (via ProcessLabel() in
qemuProcessHook()).
This patch addresses the change by making GenLabel() load the AppArmor
profile into the kernel after the label (profile name) is generated.
SetAllLabel() is then adjusted to only reload_profile() and append stdin_fn to
the profile when it is specified. This also makes the AppArmor driver work
like its SELinux counterpart with regard to SetAllLabel() and stdin_fn.
Bug-Ubuntu: https://launchpad.net/bugs/801569
During a savevm operation, libvirt will now use fd migration if qemu
supports it. When the AppArmor driver is enabled, AppArmorSetFDLabel()
is used but since this function simply returns '0', the dynamic AppArmor
profile is not updated and AppArmor blocks access to the save file. This
patch implements AppArmorSetFDLabel() to get the pathname of the file by
resolving the fd symlink in /proc, and then gives that pathname to
reload_profile(), which fixes 'virsh save' when AppArmor is enabled.
Reference: https://launchpad.net/bugs/795800
This fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=702044https://bugzilla.redhat.com/show_bug.cgi?id=709454
Both of these complain of a failure to use an image file that resides
on a read-only NFS volume. The function in the DAC security driver
that chowns image files to the qemu user:group before using them
already has special cases to ignore failure of chown on read-only file
systems, and in a few other cases, but it hadn't been checking for
EINVAL, which is what is returned if the qemu user doesn't even exist
on the NFS server.
Since the explanation of EINVAL in the chown man page almost exactly
matches the log message already present for the case of EOPNOTSUPP,
I've just added EINVAL to that same conditional.
Regression introduced in commit d6623003 (v0.8.8) - using the
wrong sizeof operand meant that security manager private data
was overlaying the allowDiskFormatProbing member of struct
_virSecurityManager. This reopens disk probing, which was
supposed to be prevented by the solution to CVE-2010-2238.
* src/security/security_manager.c
(virSecurityManagerGetPrivateData): Use correct offset.
When setting up a FIFO for QEMU, it allows either a pair
of fifos used unidirectionally, or a single fifo used
bidirectionally. Look for the bidirectional fifo first
when labelling since that is more useful
* src/security/security_dac.c,
src/security/security_selinux.c: Fix fifo handling
We already have virAsprintf, so picking a similar name helps for
seeing a similar purpose. Furthermore, the prefix V before printf
generally implies 'va_list', even though this variant was '...', and
the old name got in the way of adding a new va_list version.
global rename performed with:
$ git grep -l virBufferVSprintf \
| xargs -L1 sed -i 's/virBufferVSprintf/virBufferAsprintf/g'
then revert the changes in ChangeLog-old.
