[PATCH] KVM: remove printing of token address

[PATCH] KVM: remove printing of token address

[Tobin C. Harding]

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.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 virt/kvm/eventfd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index f2ac53ab8243..b54503ed60bd 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -416,8 +416,7 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
 		irqfd->consumer.start = kvm_arch_irq_bypass_start;
 		ret = irq_bypass_register_consumer(&irqfd->consumer);
 		if (ret)
-			pr_info("irq bypass consumer (token %p) registration fails: %d\n",
-				irqfd->consumer.token, ret);
+			pr_info("irq bypass consumer registration fails: %d\n", ret);
 	}
 #endif
 
-- 
2.7.4

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

[Paolo Bonzini]

----- 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.  Also, please do the same change on the VFIO
side (drivers/vfio/pci/vfio_pci_intrs.c, call to irq_bypass_register_producer).

Thanks,

Paolo

> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> ---
>  virt/kvm/eventfd.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index f2ac53ab8243..b54503ed60bd 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -416,8 +416,7 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
>  		irqfd->consumer.start = kvm_arch_irq_bypass_start;
>  		ret = irq_bypass_register_consumer(&irqfd->consumer);
>  		if (ret)
> -			pr_info("irq bypass consumer (token %p) registration fails: %d\n",
> -				irqfd->consumer.token, ret);
> +			pr_info("irq bypass consumer registration fails: %d\n", ret);
>  	}
>  #endif
>  
> --
> 2.7.4
> 
> 

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

[Tobin C. Harding]

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.

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

Oh, cool. I was wondering where the other side was. Will send v2

thanks,
Tobin.

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

[Paolo Bonzini]

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.

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).
> 
> Oh, cool. I was wondering where the other side was. Will send v2
> 
> thanks,
> Tobin.
> 

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

[Tobin C. Harding]

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.

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

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

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