On Tue, 2016-12-06 at 10:17 -0600, Jamie Strandboge wrote:

On Mon, 2016-12-05 at 17:30 +0100, Christian Ehrhardt wrote:

...

On Mon, Dec 5, 2016 at 12:21 PM, intrigeri <intrigeri+libvirt@boum.org> wrote:

...

+  @{PROC}/@{pid}/task/@{tid}/comm rw,

Hi, we have used the following for now that we planned to submit soon: owner @{PROC}/@{pid}/task/[0-9]*/comm rw

But I like yours more since you are adding the explicit TID instead of a pattern. I'm convinced you confirmed your fix working, but I wonder if might want to consider the "owner" part we had.

Note that @{pid} and @{tid} may be causing some confusion. Today, these are simple pattern matches in anticipation of AppArmor kernel variables. From /etc/apparmor.d/tunables/kernelvars:

# until kernel vars are implemented # and until the parser supports nested groupings like #   @{pid}=[1-9]{[0-9]{[0-9]{[0-9]{[0-9]{[0-9],},},},},} # use @{pid}={[1-9],[1-9][0-9],[1-9][0-9][0-9],[1-9][0-9][0-9][0-9],[1-9][0-9][0- 9][0- 9][0-9],[1-9][0-9][0-9][0-9][0-9][0-9]}

#same pattern as @{pid} for now @{tid}=@{pid}

As such, while intrigeri's use of @{tid} is more correct for the long term, it is not (appreciably) different from Christian's use of [0-9]*.

This means that either rule will allow modifying other process' 'comm' value. Since libvirt is *usually* configured to launch VMs as non-root (eg. the 'libvirt-qemu' group on Debian and its derivatives) there is some mitigation with owner match, but it does mean that any libvirt managed qemu process can update the 'comm' value of any other libvirt managed qemu process. For systems that are configured to launch qemu under root, then the rule means a qemu process is able to modify the 'comm' value of any other root process. Either way, this breaks qemu isolation and as a result I think it is wrong to add it to the libvirt-qemu abstraction until kernel variables are available.

The best we can do at this point is conditionally add the owner rule per VM or per invocation of the command that runs set_process_name. The safest would be to add the rule when set_process_name is called, then remove it after with a profile reload. I'm unfamiliar with set_process_name and when it is run so I can't say if this is a viable approach or not.

If it is not possible to update the profile in this manner, then at a minimum there should be a FIXME comment above this rule discussing all of this.

I forgot to reiterate: the above is true *unless* there is another non-DAC, non- MAC kernel mediation (eg, does the kernel only allow modifying the 'comm' value of its own threads? If so, then the rule would be safe to add to the default abstraction (though we should document that it is safe)). -- Jamie Strandboge | http://www.canonical.com

On Wed, 2016-12-07 at 08:37 +0100, Christian Ehrhardt wrote:

On Tue, Dec 6, 2016 at 5:40 PM, Jamie Strandboge <jamie@canonical.com> wrote:

...

I forgot to reiterate: the above is true *unless* there is another non-DAC, non- MAC kernel mediation (eg, does the kernel only allow modifying the 'comm' value of its own threads? If so, then the rule would be safe to add to the default abstraction (though we should document that it is safe)).

Thanks for your help Jamie on thinking through the implications of this - I really highly appreciate! For the given interface the v2 should be safe see e.g. http://man7.org/linux/man-pages/man5/proc.5.html Quoting from there: "... A thread may modify *its* comm value, or that of any of other thread *in the same thread group* ..."

Thanks for investigating this. +1 on adding this it the libvirt-qemu abstraction: # Per man(5) proc, the kernel enforces that a thread may # only modify its comm value or those in its thread group. owner @{PROC}/@{pid}/task/@{tid}/comm rw, -- Jamie Strandboge | http://www.canonical.com

Hi, Daniel P. Berrange:

On Mon, Dec 12, 2016 at 04:04:34PM +0100, Martin Kletzander wrote:

...

Didn't we have a policy of using real names in commit messages? I remember someone advocating that (Eric?), so I did that as well. But to be honest, I can't find it anywhere in our docs, but it makes sense if there is a need for anything related to attributions or copyrights.

I just assumed "intrigeri" is a real name :-)

I have no ID with "intrigeri" written on it, so you may consider it's not a "real name". However, it is a "real name" in the sense that I've been known under this name, and only it, in the FOSS community since ~13 years. A number of list members have met me at various events such as FOSDEM or DebConf. This was enough for me to become a Debian developer as "intrigeri" a few years ago; AFAIK the Debian project has granted two such exceptions like this in its history, based on good reasons for it, that one has to (privately) explain to the Debian accounts managers… which was the case for me. This was also enough for many people to feel comfortable publicly signing my OpenPGP key, after working with me for some time, remotely and in person. I hope that this background will be enough to allow me to keep contributing to the libvirt project using the same identity as the one I'm known as, and was able to contribute as, everywhere else in the FOSS community so far. If a newly formalized policy prevents me from contributing as "intrigeri", then I'll need to have my contributions relayed by other people to this project, which will be impractical, demotivating, and a waste of time for everybody involved. In case this helps:

In this case the patches are the same as those already carried by Ubuntu, and trivial enough to not have copyright consequences imho.

I am not a C person, so I expect that all my future contributions will be minor updates to the AppArmor policy for libvirt. They should thus not have copyright consequences :) Thanks for your work, cheers! -- intrigeri