Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 3/3] virt: Add sev_secret module to expose confidential
 computing secrets
To:     Andrew Scull <ascull@google.com>, Ard Biesheuvel <ardb@kernel.org>
Cc:     linux-efi <linux-efi@vger.kernel.org>,
        Borislav Petkov <bp@suse.de>,
        Ashish Kalra <ashish.kalra@amd.com>,
        Brijesh Singh <brijesh.singh@amd.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        James Morris <jmorris@namei.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Andi Kleen <ak@linux.intel.com>,
        "Dr. David Alan Gilbert" <dgilbert@redhat.com>,
        James Bottomley <jejb@linux.ibm.com>,
        Tobin Feldman-Fitzthum <tobin@linux.ibm.com>,
        Jim Cadden <jcadden@ibm.com>, linux-coco@lists.linux.dev,
        linux-security-module@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Dov Murik <dovmurik@linux.ibm.com>
References: <20210809190157.279332-1-dovmurik@linux.ibm.com>
 <20210809190157.279332-4-dovmurik@linux.ibm.com>
 <YRZuIIVIzMfgjtEl@google.com>
 <CAMj1kXFC-cizTw2Tv40uZHdLArKtdMNxdQXWoPWSL-8qexdkLQ@mail.gmail.com>
 <CADcWuH0mP+e6GxkUGN3ni_Yu0z8YTn-mo677obH+p-OFCL+wOQ@mail.gmail.com>
From:   Dov Murik <dovmurik@linux.ibm.com>
Message-ID: <b3c65f9d-5fd3-22c5-cd23-481774d92222@linux.ibm.com>
Date:   Fri, 20 Aug 2021 21:36:46 +0300
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <CADcWuH0mP+e6GxkUGN3ni_Yu0z8YTn-mo677obH+p-OFCL+wOQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 19/08/2021 16:02, Andrew Scull wrote:
> On Mon, 16 Aug 2021 at 10:57, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Fri, 13 Aug 2021 at 15:05, Andrew Scull <ascull@google.com> wrote:
>>>
>>> On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote:

[...]

>>>
>>>> +static int sev_secret_unlink(struct inode *dir, struct dentry *dentry)
>>>> +{
>>>> +     struct sev_secret *s = sev_secret_get();
>>>> +     struct inode *inode = d_inode(dentry);
>>>> +     struct secret_entry *e = (struct secret_entry *)inode->i_private;
>>>> +     int i;
>>>> +
>>>> +     if (e) {
>>>> +             /* Zero out the secret data */
>>>> +             memzero_explicit(e->data, secret_entry_data_len(e));
>>>
>>> Would there be a benefit in flushing these zeros?
>>>
>>
>> Do you mean cache clean+invalidate? Better to be precise here.
> 
> At least a clean, to have the zeros written back to memory from the
> cache, in order to overwrite the secret.
> 

I agree, but not sure how to implement this:

I see there's an arch_wb_cache_pmem exported function which internally
(in arch/x86/lib/usercopy_64.c) calls clean_cache_range which seems to
do what we want (assume the secret can be longer than the cache line).

But arch_wb_cache_pmem is declared in include/linux/libnvdimm.h and
guarded with #ifdef CONFIG_ARCH_HAS_PMEM_API -- both seem not related to
what I'm trying to do.

I see there's an exported clflush_cache_range for x86 -- but that's a
clean+flush if I understand correctly.

Suggestions on how to approach? I can copy the clean_cache_range
implementation into the sev_secret module but hopefully there's a better
way to reuse.  Maybe export clean_cache_range in x86?

Since this is for SEV the solution can be x86-specific, but if there's a
generic way I guess it's better (I think all of sev_secret module
doesn't have x86-specific stuff).

-Dov


>>
>>>> +             e->guid = NULL_GUID;
>>>> +     }
>>>> +
>>>> +     inode->i_private = NULL;
>>>> +
>>>> +     for (i = 0; i < SEV_SECRET_NUM_FILES; i++)
>>>> +             if (s->fs_files[i] == dentry)
>>>> +                     s->fs_files[i] = NULL;
>>>> +
>>>> +     /*
>>>> +      * securityfs_remove tries to lock the directory's inode, but we reach
>>>> +      * the unlink callback when it's already locked
>>>> +      */
>>>> +     inode_unlock(dir);
>>>> +     securityfs_remove(dentry);
>>>> +     inode_lock(dir);
>>>> +
>>>> +     return 0;
>>>> +}