On Mon, 2016-12-05 at 17:30 +0100, Christian Ehrhardt wrote: > > On Mon, Dec 5, 2016 at 12:21 PM, intrigeri <intrigeri+libvirt(a)boum.org&gt; > 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.

On Tue, Dec 6, 2016 at 5:40 PM, Jamie Strandboge <jamie(a)canonical.com&gt; 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* ..."

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 :-)

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