Re: [PATCH] KVM: remove printing of token address

[Tobin C. Harding]

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.