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.
This is the part I'm stuck on as to why - based on the previous
assertion. I'm just not visualizing all those various chardev configs.
Just so you know - I have to be out Friday, but will be back Monday. If
you get someone else to ACK this one in the mean time - that's fine.
John
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