linux-kernel - Re: [PATCH v4 06/14] KVM: s390: pv: properly handle page flags for protected guests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210917165735.1e86d02b@p-imbrenda>
Date:   Fri, 17 Sep 2021 16:57:35 +0200
From:   Claudio Imbrenda <imbrenda@...ux.ibm.com>
To:     Christian Borntraeger <borntraeger@...ibm.com>
Cc:     kvm@...r.kernel.org, cohuck@...hat.com, frankja@...ux.ibm.com,
        thuth@...hat.com, pasic@...ux.ibm.com, david@...hat.com,
        linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        Ulrich.Weigand@...ibm.com,
        Gerald Schaefer <gerald.schaefer@...ux.ibm.com>
Subject: Re: [PATCH v4 06/14] KVM: s390: pv: properly handle page flags for
 protected guests

On Mon, 6 Sep 2021 18:16:10 +0200
Christian Borntraeger <borntraeger@...ibm.com> wrote:

> On 06.09.21 17:56, Claudio Imbrenda wrote:
> > On Mon, 6 Sep 2021 17:46:40 +0200
> > Christian Borntraeger <borntraeger@...ibm.com> wrote:
> >   
> >> On 18.08.21 15:26, Claudio Imbrenda wrote:  
> >>> Introduce variants of the convert and destroy page functions that also
> >>> clear the PG_arch_1 bit used to mark them as secure pages.
> >>>
> >>> These new functions can only be called on pages for which a reference
> >>> is already being held.
> >>>
> >>> Signed-off-by: Claudio Imbrenda <imbrenda@...ux.ibm.com>
> >>> Acked-by: Janosch Frank <frankja@...ux.ibm.com>  
> >>
> >> Can you refresh my mind? We do have over-indication of PG_arch_1 and this
> >> might result in spending some unneeded cycles but in the end this will be
> >> correct. Right?
> >> And this patch will fix some unnecessary places that add overindication.  
> > 
> > correct, PG_arch_1 will still overindicate, but with this patch it will
> > happen less.
> > 
> > And PG_arch_1 overindication is perfectly fine from a correctness point
> > of view.  
> 
> Maybe add something like this to the patch description then.
> 
> >>> +/*
> >>> + * The caller must already hold a reference to the page
> >>> + */
> >>> +int uv_destroy_owned_page(unsigned long paddr)
> >>> +{
> >>> +	struct page *page = phys_to_page(paddr);  
> 
> Do we have to protect against weird mappings without struct page here? I have not
> followed the discussion about this topic. Maybe Gerald knows if we can have memory
> without struct pages.

at first glance, it seems we can't have mappings without a struct page

> 
> >>> +	int rc;
> >>> +
> >>> +	get_page(page);
> >>> +	rc = uv_destroy_page(paddr);
> >>> +	if (!rc)
> >>> +		clear_bit(PG_arch_1, &page->flags);
> >>> +	put_page(page);
> >>> +	return rc;
> >>> +}
> >>> +
> >>>    /*
> >>>     * Requests the Ultravisor to encrypt a guest page and make it
> >>>     * accessible to the host for paging (export).
> >>> @@ -154,6 +170,22 @@ int uv_convert_from_secure(unsigned long paddr)
> >>>    	return 0;
> >>>    }
> >>>    
> >>> +/*
> >>> + * The caller must already hold a reference to the page
> >>> + */
> >>> +int uv_convert_owned_from_secure(unsigned long paddr)
> >>> +{
> >>> +	struct page *page = phys_to_page(paddr);  
> 
> Same here. If this is not an issue (and you add something to the patch description as
> outlined above)
> 
> Reviewed-by: Christian Borntraeger <borntraeger@...ibm.com>
> 
>