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

["Tobin C. Harding" <me@tobin.cc>]

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@tobin.cc>
> >>> To: "Paolo Bonzini" <pbonzini@redhat.com>, rkrcmar@redhat.com
> >>> Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, "Tobin C. Harding" <me@tobin.cc>
> >>> 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. 

Would you be happy if instead of printing the pointer we printed a unique identifier (some suitable
hash of the pointer value)? I'm working on that patch ATM, if this sounds ok I'll leave this patch
until unique identifiers are implemented.

But if we really want the address here I can put in the patch using %pK. What is your thoughts?

> Thanks,
> 
> Paolo
> 
> >> Also, please do the same change on the VFIO
> >> side (drivers/vfio/pci/vfio_pci_intrs.c, call to irq_bypass_register_producer).

And same here obviously.

thanks,
Tobin.