From: Serge Hallyn <serge.hallyn@ubuntu.com>
Allows owner access to hugepage mounts (both, the old and
new systemd variant).
Bug-Ubuntu: https://bugs.launchpad.net/bugs/1250216
Bug-Ubuntu: https://bugs.launchpad.net/bugs/1524737
Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
examples/apparmor/libvirt-qemu | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index 912b4ac..bb30530 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -184,6 +184,10 @@
/sys/bus/ r,
/sys/class/ r,
+ # for access to hugepages (LP: #1250216 and LP: #1524737)
+ owner "/run/hugepages/kvm/libvirt/qemu/**" rw,
+ owner "/dev/hugepages/libvirt/qemu/**" rw,
+
# for rbd
/etc/ceph/ceph.conf r,
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote: > From: Serge Hallyn <serge.hallyn@ubuntu.com> > > Allows owner access to hugepage mounts (both, the old and > new systemd variant). > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1250216 > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1524737 > > Signed-off-by: Stefan Bader <stefan.bader@canonical.com> > --- > examples/apparmor/libvirt-qemu | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/examples/apparmor/libvirt-qemu > b/examples/apparmor/libvirt-qemu > index 912b4ac..bb30530 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -184,6 +184,10 @@ > /sys/bus/ r, > /sys/class/ r, > > + # for access to hugepages (LP: #1250216 and LP: #1524737) > + owner "/run/hugepages/kvm/libvirt/qemu/**" rw, > + owner "/dev/hugepages/libvirt/qemu/**" rw, > + These rules are not vm-specific. I'm not familiar with the hugepages feature in libvirt, but the rule suggests that libvirtd will create files in these directories per-vm, and then the vm uses what libvirtd created for it. I see virSecurityManagerSetHugepages() in security_manager.c, is it possible that these rules can be removed and vm-specific ones added dynamically with virt-aa-helper? -- Jamie Strandboge | http://www.canonical.com-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 19, 2017 at 5:21 PM, Jamie Strandboge <jamie@canonical.com> wrote:
> On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote:
>> From: Serge Hallyn <serge.hallyn@ubuntu.com>
>>
>> Allows owner access to hugepage mounts (both, the old and
>> new systemd variant).
>>
>> Bug-Ubuntu: https://bugs.launchpad.net/bugs/1250216
>> Bug-Ubuntu: https://bugs.launchpad.net/bugs/1524737
>>
>> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
>> ---
>> examples/apparmor/libvirt-qemu | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/examples/apparmor/libvirt-qemu
>> b/examples/apparmor/libvirt-qemu
>> index 912b4ac..bb30530 100644
>> --- a/examples/apparmor/libvirt-qemu
>> +++ b/examples/apparmor/libvirt-qemu
>> @@ -184,6 +184,10 @@
>> /sys/bus/ r,
>> /sys/class/ r,
>>
>> + # for access to hugepages (LP: #1250216 and LP: #1524737)
>> + owner "/run/hugepages/kvm/libvirt/qemu/**" rw,
>> + owner "/dev/hugepages/libvirt/qemu/**" rw,
>> +
>
> These rules are not vm-specific. I'm not familiar with the hugepages
> feature in libvirt, but the rule suggests that libvirtd will create
> files in these directories per-vm, and then the vm uses what libvirtd
> created for it. I see virSecurityManagerSetHugepages() in
> security_manager.c,
>
> is it possible that these rules can be removed and
> vm-specific ones added dynamically with virt-aa-helper?
I agree - and in general it would also be nice to do a per guest rule
with a full path to keep the guests away from each other.
I see different ways to go thou:
#1 - per security_apparmor.c
The particular function you mention was generalized by [1] and further
reused in [3].
The creation of the per guest paths is in [2].
So essentially it comes down to a call to domainSetPathLabel with the
path as an argument.
But then security_apparmor.c so far does not implement
domainSetPathLabel at all.
If we add a function for domainSetPathLabel it could call
reload_profile with the path (as others in security_apparmor.c do).
But this path is in use by other places as well:
- qemuDomainWriteMasterKeyFile
- qemuProcessMakeDir
OTOH domainSetPathLabel is used in those places for the reson of it's
meaning "add this to the scope of the security label right?"
So we might after all want to add all these anyway?
If that is so, an (untested) change could be like:
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -953,6 +953,13 @@ AppArmorSetSavedStateLabel(virSecurityManagerPtr mgr,
return reload_profile(mgr, def, savefile, true);
}
+static int
+AppArmorSetPathLabel(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ const char *path)
+{
+ return reload_profile(mgr, def, path, true);
+}
static int
AppArmorRestoreSavedStateLabel(virSecurityManagerPtr mgr,
@@ -1045,6 +1052,8 @@ virSecurityDriver virAppArmorSecurityDriver = {
.domainSetSavedStateLabel = AppArmorSetSavedStateLabel,
.domainRestoreSavedStateLabel = AppArmorRestoreSavedStateLabel,
+ .domainSetPathLabel = AppArmorSetPathLabel,
+
.domainSetSecurityImageFDLabel = AppArmorSetFDLabel,
.domainSetSecurityTapFDLabel = AppArmorSetFDLabel,
#2 in virt-aa-helper with XML parsing
The next option would to try to detect if hugepages are used from the
xml description in virt-aa-helper.
#3 unconditional per domain rule
And finally we could avoid too much (?over?)engineering and
unconditionally add this in virt-aa-helper (at the cost of breaking if
paths generated [2] ever change):
owner "/dev/hugepages/libvirt/qemu/<domainName>" rw,
IMHO - [1] or [3] - please let me know your opinions before I follow
any of those paths.
[1]: https://libvirt.org/git/?p=libvirt.git;a=commit;h=ce937d37105c2fc95638f4dc0d2993ae5275b464
[2]: https://libvirt.org/git/?p=libvirt.git;a=commit;h=f55afd83b1338e17eae7ec83b792a7fc962edbc3
[3]: https://libvirt.org/git/?p=libvirt.git;a=commit;h=eff2b2edb147572b6d8f701f4b4f4a69580e17c8
[4]: https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/security/virt-aa-helper.c;h=07ece730e27d2be4be92adcabb54d788825659f0;hb=HEAD#l1356
--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2017-12-20 at 14:43 +0100, Christian Ehrhardt wrote:
> On Tue, Dec 19, 2017 at 5:21 PM, Jamie Strandboge <jamie@canonical.co
> m> wrote:
> > On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote:
> > > From: Serge Hallyn <serge.hallyn@ubuntu.com>
> > >
> > > Allows owner access to hugepage mounts (both, the old and
> > > new systemd variant).
> > >
> > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1250216
> > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1524737
> > >
> > > Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> > > ---
> > > examples/apparmor/libvirt-qemu | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/examples/apparmor/libvirt-qemu
> > > b/examples/apparmor/libvirt-qemu
> > > index 912b4ac..bb30530 100644
> > > --- a/examples/apparmor/libvirt-qemu
> > > +++ b/examples/apparmor/libvirt-qemu
> > > @@ -184,6 +184,10 @@
> > > /sys/bus/ r,
> > > /sys/class/ r,
> > >
> > > + # for access to hugepages (LP: #1250216 and LP: #1524737)
> > > + owner "/run/hugepages/kvm/libvirt/qemu/**" rw,
> > > + owner "/dev/hugepages/libvirt/qemu/**" rw,
> > > +
> >
> > These rules are not vm-specific. I'm not familiar with the
> > hugepages
> > feature in libvirt, but the rule suggests that libvirtd will create
> > files in these directories per-vm, and then the vm uses what
> > libvirtd
> > created for it. I see virSecurityManagerSetHugepages() in
> > security_manager.c,
> >
> > is it possible that these rules can be removed and
> > vm-specific ones added dynamically with virt-aa-helper?
>
> I agree - and in general it would also be nice to do a per guest rule
> with a full path to keep the guests away from each other.
> I see different ways to go thou:
>
> #1 - per security_apparmor.c
> The particular function you mention was generalized by [1] and
> further
> reused in [3].
> The creation of the per guest paths is in [2].
>
> So essentially it comes down to a call to domainSetPathLabel with the
> path as an argument.
> But then security_apparmor.c so far does not implement
> domainSetPathLabel at all.
>
> If we add a function for domainSetPathLabel it could call
> reload_profile with the path (as others in security_apparmor.c do).
> But this path is in use by other places as well:
> - qemuDomainWriteMasterKeyFile
> - qemuProcessMakeDir
>
> OTOH domainSetPathLabel is used in those places for the reson of it's
> meaning "add this to the scope of the security label right?"
> So we might after all want to add all these anyway?
>
> If that is so, an (untested) change could be like:
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -953,6 +953,13 @@ AppArmorSetSavedStateLabel(virSecurityManagerPtr
> mgr,
> return reload_profile(mgr, def, savefile, true);
> }
>
> +static int
> +AppArmorSetPathLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + const char *path)
> +{
> + return reload_profile(mgr, def, path, true);
> +}
>
> static int
> AppArmorRestoreSavedStateLabel(virSecurityManagerPtr mgr,
> @@ -1045,6 +1052,8 @@ virSecurityDriver virAppArmorSecurityDriver = {
> .domainSetSavedStateLabel = AppArmorSetSavedStateLabel,
> .domainRestoreSavedStateLabel =
> AppArmorRestoreSavedStateLabel,
>
> + .domainSetPathLabel = AppArmorSetPathLabel,
> +
> .domainSetSecurityImageFDLabel = AppArmorSetFDLabel,
> .domainSetSecurityTapFDLabel = AppArmorSetFDLabel,
>
>
> #2 in virt-aa-helper with XML parsing
> The next option would to try to detect if hugepages are used from the
> xml description in virt-aa-helper.
>
>
> #3 unconditional per domain rule
> And finally we could avoid too much (?over?)engineering and
> unconditionally add this in virt-aa-helper (at the cost of breaking
> if
> paths generated [2] ever change):
> owner "/dev/hugepages/libvirt/qemu/<domainName>" rw,
>
>
> IMHO - [1] or [3] - please let me know your opinions before I follow
> any of those paths.
>
> [1]: https://libvirt.org/git/?p=libvirt.git;a=commit;h=ce937d37105c2f
>
Or a combination of 2 and 3. Ie, if hugepages is enabled, add the rule
in 3.
To me, 1 feels most correct cause while the other two fix hugepages,
there seem to be lurking bugs since we aren't implementing
domainSetPathLabel.
--
Jamie Strandboge | http://www.canonical.com--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Dec 20, 2017 at 3:34 PM, Jamie Strandboge <jamie@canonical.com> wrote:
> On Wed, 2017-12-20 at 14:43 +0100, Christian Ehrhardt wrote:
>> On Tue, Dec 19, 2017 at 5:21 PM, Jamie Strandboge <jamie@canonical.co
>> m> wrote:
>> > On Tue, 2017-12-19 at 16:03 +0100, Christian Ehrhardt wrote:
>> > > From: Serge Hallyn <serge.hallyn@ubuntu.com>
>> > >
>> > > Allows owner access to hugepage mounts (both, the old and
>> > > new systemd variant).
>> > >
>> > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1250216
>> > > Bug-Ubuntu: https://bugs.launchpad.net/bugs/1524737
>> > >
>> > > Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
>> > > ---
>> > > examples/apparmor/libvirt-qemu | 4 ++++
>> > > 1 file changed, 4 insertions(+)
>> > >
>> > > diff --git a/examples/apparmor/libvirt-qemu
>> > > b/examples/apparmor/libvirt-qemu
>> > > index 912b4ac..bb30530 100644
>> > > --- a/examples/apparmor/libvirt-qemu
>> > > +++ b/examples/apparmor/libvirt-qemu
>> > > @@ -184,6 +184,10 @@
>> > > /sys/bus/ r,
>> > > /sys/class/ r,
>> > >
>> > > + # for access to hugepages (LP: #1250216 and LP: #1524737)
>> > > + owner "/run/hugepages/kvm/libvirt/qemu/**" rw,
>> > > + owner "/dev/hugepages/libvirt/qemu/**" rw,
>> > > +
>> >
>> > These rules are not vm-specific. I'm not familiar with the
>> > hugepages
>> > feature in libvirt, but the rule suggests that libvirtd will create
>> > files in these directories per-vm, and then the vm uses what
>> > libvirtd
>> > created for it. I see virSecurityManagerSetHugepages() in
>> > security_manager.c,
>> >
>> > is it possible that these rules can be removed and
>> > vm-specific ones added dynamically with virt-aa-helper?
>>
>> I agree - and in general it would also be nice to do a per guest rule
>> with a full path to keep the guests away from each other.
>> I see different ways to go thou:
>>
>> #1 - per security_apparmor.c
>> The particular function you mention was generalized by [1] and
>> further
>> reused in [3].
>> The creation of the per guest paths is in [2].
>>
>> So essentially it comes down to a call to domainSetPathLabel with the
>> path as an argument.
>> But then security_apparmor.c so far does not implement
>> domainSetPathLabel at all.
>>
>> If we add a function for domainSetPathLabel it could call
>> reload_profile with the path (as others in security_apparmor.c do).
>> But this path is in use by other places as well:
>> - qemuDomainWriteMasterKeyFile
>> - qemuProcessMakeDir
>>
>> OTOH domainSetPathLabel is used in those places for the reson of it's
>> meaning "add this to the scope of the security label right?"
>> So we might after all want to add all these anyway?
>>
>> If that is so, an (untested) change could be like:
>> --- a/src/security/security_apparmor.c
>> +++ b/src/security/security_apparmor.c
>> @@ -953,6 +953,13 @@ AppArmorSetSavedStateLabel(virSecurityManagerPtr
>> mgr,
>> return reload_profile(mgr, def, savefile, true);
>> }
>>
>> +static int
>> +AppArmorSetPathLabel(virSecurityManagerPtr mgr,
>> + virDomainDefPtr def,
>> + const char *path)
>> +{
>> + return reload_profile(mgr, def, path, true);
>> +}
>>
>> static int
>> AppArmorRestoreSavedStateLabel(virSecurityManagerPtr mgr,
>> @@ -1045,6 +1052,8 @@ virSecurityDriver virAppArmorSecurityDriver = {
>> .domainSetSavedStateLabel = AppArmorSetSavedStateLabel,
>> .domainRestoreSavedStateLabel =
>> AppArmorRestoreSavedStateLabel,
>>
>> + .domainSetPathLabel = AppArmorSetPathLabel,
>> +
>> .domainSetSecurityImageFDLabel = AppArmorSetFDLabel,
>> .domainSetSecurityTapFDLabel = AppArmorSetFDLabel,
>>
>>
>> #2 in virt-aa-helper with XML parsing
>> The next option would to try to detect if hugepages are used from the
>> xml description in virt-aa-helper.
>>
>>
>> #3 unconditional per domain rule
>> And finally we could avoid too much (?over?)engineering and
>> unconditionally add this in virt-aa-helper (at the cost of breaking
>> if
>> paths generated [2] ever change):
>> owner "/dev/hugepages/libvirt/qemu/<domainName>" rw,
>>
>>
>> IMHO - [1] or [3] - please let me know your opinions before I follow
>> any of those paths.
>>
>> [1]: https://libvirt.org/git/?p=libvirt.git;a=commit;h=ce937d37105c2f
>>
>
> Or a combination of 2 and 3. Ie, if hugepages is enabled, add the rule
> in 3.
>
> To me, 1 feels most correct cause while the other two fix hugepages,
> there seem to be lurking bugs since we aren't implementing
> domainSetPathLabel.
>
I work on #1 a while and I think we can do a lot good here.
Yet while I'm convinced at the changes this is currently a debugging nightmare.
I guess it wants to become a 2018 submission.
So for now keep this hugepage patch out of consideration when looking
at applying all those with many +1's.
I'll hopefully come back somewhen in 2018 with a dynamic handling of
hugepages (and several more actually).
> --
> Jamie Strandboge | http://www.canonical.com
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[...] >> To me, 1 feels most correct cause while the other two fix hugepages, >> there seem to be lurking bugs since we aren't implementing >> domainSetPathLabel. >> > > I work on #1 a while and I think we can do a lot good here. > Yet while I'm convinced at the changes this is currently a debugging nightmare. > I guess it wants to become a 2018 submission. Note: I'll not user reply-to onto this thread to keep threading somewhat sane. Also the new submission, while inspired by this discussion, is a totally separate thing. > So for now keep this hugepage patch out of consideration when looking > at applying all those with many +1's. So as I expected the hugepage patch of this series will be covered by the new series that I submit. But I wanted to ask for all the others changes in the current series here to be considered - most have one or two acks already. Let me know if more is needed to commit them. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2018-01-03 at 17:55 +0100, Christian Ehrhardt wrote: > [...] > > > > To me, 1 feels most correct cause while the other two fix hugepages, > > > there seem to be lurking bugs since we aren't implementing > > > domainSetPathLabel. > > > > > > > I work on #1 a while and I think we can do a lot good here. > > Yet while I'm convinced at the changes this is currently a debugging nightmare. > > I guess it wants to become a 2018 submission. > > Note: I'll not user reply-to onto this thread to keep threading somewhat sane. > Also the new submission, while inspired by this discussion, is a > totally separate thing. > > > So for now keep this hugepage patch out of consideration when looking > > at applying all those with many +1's. > > So as I expected the hugepage patch of this series will be covered by > the new series that I submit. > But I wanted to ask for all the others changes in the current series > here to be considered - most have one or two acks already. > Let me know if more is needed to commit them. This is the only patch from the series that I haven't pushed or ignored... What should be done with this one? -- Cedric > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jan 9, 2018 at 11:02 AM, Cedric Bosdonnat <cbosdonnat@suse.com> wrote: > On Wed, 2018-01-03 at 17:55 +0100, Christian Ehrhardt wrote: >> [...] >> >> > > To me, 1 feels most correct cause while the other two fix hugepages, >> > > there seem to be lurking bugs since we aren't implementing >> > > domainSetPathLabel. >> > > >> > >> > I work on #1 a while and I think we can do a lot good here. >> > Yet while I'm convinced at the changes this is currently a debugging nightmare. >> > I guess it wants to become a 2018 submission. >> >> Note: I'll not user reply-to onto this thread to keep threading somewhat sane. >> Also the new submission, while inspired by this discussion, is a >> totally separate thing. >> >> > So for now keep this hugepage patch out of consideration when looking >> > at applying all those with many +1's. >> >> So as I expected the hugepage patch of this series will be covered by >> the new series that I submit. >> But I wanted to ask for all the others changes in the current series >> here to be considered - most have one or two acks already. >> Let me know if more is needed to commit them. > > This is the only patch from the series that I haven't pushed or ignored... > What should be done with this one? On this change here please do nothing - this is the one superseded by the new series that is in review since early january. Michael already had feedback there which I'm looking into shortly. Thank you so much for committing the acked patches of this series! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.