From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755798AbdJJIbV (ORCPT ); Tue, 10 Oct 2017 04:31:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59114 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752146AbdJJIbU (ORCPT ); Tue, 10 Oct 2017 04:31:20 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com EAB727E43F Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=pbonzini@redhat.com Subject: Re: [PATCH] KVM: remove printing of token address To: "Tobin C. Harding" Cc: rkrcmar@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org References: <1507530614-10504-1-git-send-email-me@tobin.cc> <1845029054.11118038.1507535378802.JavaMail.zimbra@redhat.com> <20171009100415.GA12785@eros> <20171009235107.GD20083@eros> From: Paolo Bonzini Message-ID: <3a2ae983-ab5c-65ae-cdfd-dd276ed1a284@redhat.com> Date: Tue, 10 Oct 2017 10:31:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20171009235107.GD20083@eros> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Tue, 10 Oct 2017 08:31:20 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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" >>>>> To: "Paolo Bonzini" , rkrcmar@redhat.com >>>>> Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, "Tobin C. Harding" >>>>> 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. >