On Thu, Jun 15, 2017 at 07:57:18AM -0400, John Ferlan wrote:
On 06/15/2017 03:11 AM, Pavel Hrdina wrote:
> On Tue, Jun 13, 2017 at 08:00:41PM -0400, John Ferlan wrote:
>>
>>
>> On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
>>> In the case that virtlogd is used as stdio handler we pass to QEMU
>>> only FD to a PIPE connected to virtlogd instead of the file itself.
>>>
>>> Resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=1430988
>>>
>>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>>> ---
>>>
>>> Notes:
>>> new in v2
>>>
>>> src/lxc/lxc_process.c | 6 ++---
>>> src/qemu/qemu_security.c | 9 +++++--
>>> src/security/security_apparmor.c | 7 ++++--
>>> src/security/security_dac.c | 54
+++++++++++++++++++++++++++++++---------
>>> src/security/security_driver.h | 6 +++--
>>> src/security/security_manager.c | 12 ++++++---
>>> src/security/security_manager.h | 6 +++--
>>> src/security/security_nop.c | 6 +++--
>>> src/security/security_selinux.c | 53
++++++++++++++++++++++++++++++---------
>>> src/security/security_stack.c | 12 ++++++---
>>> tests/securityselinuxlabeltest.c | 2 +-
>>> 11 files changed, 127 insertions(+), 46 deletions(-)
>>>
>>
>> Why is it a (!chr_seclabel && chardevStdioLogd)? More to the point why
>> is (!chr_seclabel) even matter?
>
> If you configure the label we shouldn't ignore it in some cases, that's
> just wrong. If the label is set for the char device we will configure
> it every time even if it will fail to start the guest, it's a
> responsibility of the user to configure proper label if it is provided.
>
When email doesn't convey the question... Ugh... I'm also trying to
speed learn an area of the code and review at the same time.
If I go back to commit id 'f8b08d0e' where the original implementation
to add labels for chardevs was done (but has been modified for patch 1
to change where the label is stored), I get the impression that a label
should be added either from something specifically supplied for the
<chardev> or to use the per domain one:
"The source element may contain an optional seclabel to override the way
that labelling is done on the socket path. If this element is not
present, the security label is inherited from the per-domain setting."
So if I look at the condition "(!chr_seclabel && chardevStdioLogd)"
added by this patch to decide whether or not to supply the label, I'm
left with the impression that if for this particular chardev a label
doesn't exist *and* the configuration option chardevStdioLogd is true,
then we're going to return happy status *and* not inherit the per-domain
setting.
So the bug is then that applying the default domain label for a chardev
configured to use a stdio handle is incorrect? Perhaps I didn't get
that from reading bz or the patch.
Yes, that's the bug. If virtlogd is used to handle stdio for char
devices we shouldn't relabel the @path to the default labels, the
@path doesn't have to be accessible by the qemu process, it has to be
accessible by the virtlogd process.
>> IIUC, whether or not someone set the label for the chardev,
for this
>> particular issue/config where the chardev has a <log file=$path>, we
>> don't want to set (or restore) the label. I feel like I'm missing
>> something subtle. Maybe a bit more explanation of the adjustment would
>> help me...
>
> This is not for the <log file=$path/> but for the <source path=$path/>.
> We don't relabel $path for <log file=$path/> at all.
>
hmm.. ah, right... I kept scrolling back and forth in the bz and the
docs, but missed this in the bz:
3) Check the virtlogd.log:
error : virRotatingFileWriterEntryNew:113 : Unable to open file:
/var/log/libvirt/qemu/log: Permission denied
I guess I got lost in the power of suggestion of reading the docs
regarding the "optional log file" that can be associated paragraph and
trying to learn on the fly so that you at least get a review in a
somewhat timely manner ;-)
>> Wouldn't these changes end up selecting "any" chardev if
>> chardevStdioLogd ended up being set regardless of whether they were
>> actually using the log file?
>
> I don't know what you mean by this sentence?
>
Well let's see, chardevStdioLogd is set to true when meeting the two
conditions a qemu.conf global "cfg->stdioLogd" && a per domain or
emulator image capability "QEMU_CAPS_CHARDEV_FILE_APPEND".
So, conceivably chardevStdioLogd could be true for *any* domain as long
as those conditions are met, right?
Yes, the two conditions are checked while starting a new domain in
qemuProcessPrepareDomain() and stored in the private date of that
domain.
If you have a domain that has chardev's which are not configured
to use
the stdio handler, then the chardevStdioLogd could still be true, right?
No, if the @chardevStdioLogd is true all char devices for that domain
will use virtlogd.
The only issue I've just found out is that the code path for chardev
hot-plug isn't updated to use virtlogd when it should be so for
hot-plugged char devices we pass the path directly to QEMU.
With this patch applied the hot-plug fails if virtlogd is used because
we don't relabel the path but we pass it directly to QEMU, this needs to
be fixed to not introduce a regression, sigh.
Pavel
If that's the case and the chardev doesn't have a label
associated, then
we just return happy status and we do not inherit the per domain
setting. Wouldn't that be incorrect?
My concern is more we're making a change in a (mostly) common set of
functions for a (very) specific problem.
>> As an aside, I think there's an "oddity" when it comes to the
Restore,
>> but I'm not sure how to put it into words exactly. If a guest is running
>> code prior to this set of changes, would it have successfully run a Set?
>> If so, then after applying this change and restarting, the label
>> wouldn't be reset, right? What happens at guest shutdown - does the
>> label not get unset now? Of course this is all "interaction" with
>> virtlogd restart that really throws a monkey wrench into things.
>
> No, that's not correct. The @chardevStdioLogd is stored in the status
> XML (the one stored in /var/run/libvirt/qemu/$domain_name.xml). So when
> the libvirtd is stopped and started with this patch applied the status
> XML doesn't have the @chardevStdioLogd stored in it so it will be false
> and we will reset the label. The @chardevStdioLogd is updated only when
> the domain is started and we will store the value in the status XML
> only with new libvirtd and only in that case we will not set/restore
> the label.
>
hmmm.. Reading the bz indicates the 'virtlogd' daemon restarting... This
is where all this gets a bit "odd" for me. Like I said it was a weird
thing to even try and explain, but I think you talked me off the ledge
of concern.
John
>> Also, why is the Smartcard chardev handling not using this
>
> The smartcard doesn't ever use virtlogd as stdio handler.
>
> Pavel
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list