On Tue, Oct 10, 2017 at 10:31:11AM +0200, Paolo Bonzini wrote: > On 10/10/2017 01:51, Tobin C. Harding wrote: > > On Mon, Oct 09, 2017 at 12:58:12PM +0200, Paolo Bonzini wrote: > >> On 09/10/2017 12:04, Tobin C. Harding wrote: > >>> On Mon, Oct 09, 2017 at 03:49:38AM -0400, Paolo Bonzini wrote: > >>>> > >>>> > >>>> ----- Original Message ----- > >>>>> From: "Tobin C. Harding" <me@xxxxxxxx> > >>>>> To: "Paolo Bonzini" <pbonzini@xxxxxxxxxx>, rkrcmar@xxxxxxxxxx > >>>>> Cc: kvm@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, "Tobin C. Harding" <me@xxxxxxxx> > >>>>> Sent: Monday, October 9, 2017 8:30:14 AM > >>>>> Subject: [PATCH] KVM: remove printing of token address > >>>>> > >>>>> KVM currently prints the address of the consumer token. It is not > >>>>> immediately clear what benefit it is to see this address. Printing > >>>>> this address leaks kernel pointers into dmesg and is a security risk. > >>>>> > >>>>> Remove the consumer token address from error message output. > >>>> > >>>> It should use %pK instead. > >>> > >>> Is there any other way we can identify a token? There is some push back against kpt_restrict (as > >>> used by %pK) at the moment. If there is another sane way to do it perhaps we could consider that, > >>> else I'll use %pK for v2. > >> > >> Not really, we know it is an eventfd but you can't go from the struct > >> eventfd_ctx* (the token) to the corresponding struct file. > >> > >> I'm not sure about the pushback... I've read your name in > >> https://lwn.net/Articles/735589/ :) and that article says "the same > >> effect as a restrictive kptr_restrict setting could be achieved by > >> searching for (and fixing) every use of unadorned "%p" directives in the > >> kernel". As I understand it, the push back is against restrictive > >> kptr_restrict settings, not against using "%pK" _to avoid the need_ for > >> such a restrictive setting. > > > > In the thread that article is based on Linus airs his view that kptr_restrict is fundamentally > > broken. > > And I agree with him that more restrictive kptr_restrict settings are > kind of broken, because either no one would use them, or if you made > them the default people would start using "%x". Further there is the > issue that people are _already_ using "%x" already instead of e.g. > "%pa". So yeah, kptr_restrict does look a bit like security theater. > > However, issues with kptr_restrict do not necessarily extend to "%pK" or > "%pa". The problem is that there are too many instances of "%p", and > I'm happy that you want to fix them. Since we _do_ have kptr_restrict, > I don't see anything wrong with converting them to "%pK". > > And if everybody started using "%pK" and "%pa" appropriately, then 1) > you wouldn't need restrictive kptr_restrict settings anymore; 2) > kptr_restrict would actually prevent address leaks. > > > Would you be happy if instead of printing the pointer we printed a unique identifier (some suitable > > hash of the pointer value)? > > As long as the "suitable hash" is computed inside printk, I don't care. > If the suitable hash would be in KVM or VFIO code, then please no. :) I wouldn't do that to you :) > Paolo Thanks for you explanations Paolo. Tobin.
