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

[Paolo Bonzini]

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

Paolo


> 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.
>