On 11/12/24 13:30, Dionna Amalie Glaze wrote:
> On Tue, Nov 12, 2024 at 7:52 AM Tom Lendacky <thomas.lendacky@xxxxxxx> wrote:
>>
>> On 11/7/24 17:24, Dionna Glaze wrote:
>>> ccp now prefers all calls from external drivers to dominate all calls
>>> into the driver on behalf of a user with a successful
>>> sev_check_external_user call.
>>
>> Would it be simpler to have the new APIs take an fd for an argument,
>> instead of doing this rework?
>
> Simpler but I think worse?
> The choice of using sev_do_cmd versus __sev_issue_cmd in kvm's
> implementation is the matter of dominance of access checking.
> There's no need to check the fd in the activate function or
> decommission function. It's not needed to be checked in a loop for
> snp_launch_update.
Very true.
> I can either complete the removal of __sev_issue_cmd in this patch or
> move to make the context creation function take an fd. What do you
> think is better?
The re-work you're looking at doing is probably a patch series on its
own. I don't think you need to do all that work for this series. You
just need to be sure that each command invocation that requires the fd
check doesn't lose that in an ioctl() path for now.
Thanks,
Tom
>
>
>>
>> Thanks,
>> Tom
>>
>>>
>>> CC: Sean Christopherson <seanjc@xxxxxxxxxx>
>>> CC: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>>> CC: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>>> CC: Ingo Molnar <mingo@xxxxxxxxxx>
>>> CC: Borislav Petkov <bp@xxxxxxxxx>
>>> CC: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
>>> CC: Ashish Kalra <ashish.kalra@xxxxxxx>
>>> CC: Tom Lendacky <thomas.lendacky@xxxxxxx>
>>> CC: John Allen <john.allen@xxxxxxx>
>>> CC: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
>>> CC: "David S. Miller" <davem@xxxxxxxxxxxxx>
>>> CC: Michael Roth <michael.roth@xxxxxxx>
>>> CC: Luis Chamberlain <mcgrof@xxxxxxxxxx>
>>> CC: Russ Weight <russ.weight@xxxxxxxxx>
>>> CC: Danilo Krummrich <dakr@xxxxxxxxxx>
>>> CC: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>> CC: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
>>> CC: Tianfei zhang <tianfei.zhang@xxxxxxxxx>
>>> CC: Alexey Kardashevskiy <aik@xxxxxxx>
>>>
>>> Signed-off-by: Dionna Glaze <dionnaglaze@xxxxxxxxxx>
>>> ---
>>> arch/x86/kvm/svm/sev.c | 18 +++++++++++++++---
>>> drivers/crypto/ccp/sev-dev.c | 12 ------------
>>> include/linux/psp-sev.h | 27 ---------------------------
>>> 3 files changed, 15 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>> index d0e0152aefb32..cea41b8cdabe4 100644
>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -528,21 +528,33 @@ static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error)
>>> return ret;
>>> }
>>>
>>> -static int __sev_issue_cmd(int fd, int id, void *data, int *error)
>>> +static int sev_check_external_user(int fd)
>>> {
>>> struct fd f;
>>> - int ret;
>>> + int ret = 0;
>>>
>>> f = fdget(fd);
>>> if (!fd_file(f))
>>> return -EBADF;
>>>
>>> - ret = sev_issue_cmd_external_user(fd_file(f), id, data, error);
>>> + if (!file_is_sev(fd_file(f)))
>>> + ret = -EBADF;
>>>
>>> fdput(f);
>>> return ret;
>>> }
>>>
>>> +static int __sev_issue_cmd(int fd, int id, void *data, int *error)
>>> +{
>>> + int ret;
>>> +
>>> + ret = sev_check_external_user(fd);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return sev_do_cmd(id, data, error);
>>> +}
>>> +
>>> static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)
>>> {
>>> struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>>> index f92e6a222da8a..67f6425b7ed07 100644
>>> --- a/drivers/crypto/ccp/sev-dev.c
>>> +++ b/drivers/crypto/ccp/sev-dev.c
>>> @@ -2493,18 +2493,6 @@ bool file_is_sev(struct file *p)
>>> }
>>> EXPORT_SYMBOL_GPL(file_is_sev);
>>>
>>> -int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
>>> - void *data, int *error)
>>> -{
>>> - int rc = file_is_sev(filep) ? 0 : -EBADF;
>>> -
>>> - if (rc)
>>> - return rc;
>>> -
>>> - return sev_do_cmd(cmd, data, error);
>>> -}
>>> -EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
>>> -
>>> void sev_pci_init(void)
>>> {
>>> struct sev_device *sev = psp_master->sev_data;
>>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
>>> index ed85c0cfcfcbe..b4164d3600702 100644
>>> --- a/include/linux/psp-sev.h
>>> +++ b/include/linux/psp-sev.h
>>> @@ -860,30 +860,6 @@ int sev_platform_init(struct sev_platform_init_args *args);
>>> */
>>> int sev_platform_status(struct sev_user_data_status *status, int *error);
>>>
>>> -/**
>>> - * sev_issue_cmd_external_user - issue SEV command by other driver with a file
>>> - * handle.
>>> - *
>>> - * This function can be used by other drivers to issue a SEV command on
>>> - * behalf of userspace. The caller must pass a valid SEV file descriptor
>>> - * so that we know that it has access to SEV device.
>>> - *
>>> - * @filep - SEV device file pointer
>>> - * @cmd - command to issue
>>> - * @data - command buffer
>>> - * @error: SEV command return code
>>> - *
>>> - * Returns:
>>> - * 0 if the SEV successfully processed the command
>>> - * -%ENODEV if the SEV device is not available
>>> - * -%ENOTSUPP if the SEV does not support SEV
>>> - * -%ETIMEDOUT if the SEV command timed out
>>> - * -%EIO if the SEV returned a non-zero return code
>>> - * -%EBADF if the file pointer is bad or does not grant access
>>> - */
>>> -int sev_issue_cmd_external_user(struct file *filep, unsigned int id,
>>> - void *data, int *error);
>>> -
>>> /**
>>> * file_is_sev - returns whether a file pointer is for the SEV device
>>> *
>>> @@ -1043,9 +1019,6 @@ sev_guest_activate(struct sev_data_activate *data, int *error) { return -ENODEV;
>>>
>>> static inline int sev_guest_df_flush(int *error) { return -ENODEV; }
>>>
>>> -static inline int
>>> -sev_issue_cmd_external_user(struct file *filep, unsigned int id, void *data, int *error) { return -ENODEV; }
>>> -
>>> static inline bool file_is_sev(struct file *filep) { return false; }
>>>
>>> static inline void *psp_copy_user_blob(u64 __user uaddr, u32 len) { return ERR_PTR(-EINVAL); }
>
>
>
> --
> -Dionna Glaze, PhD, CISSP, CCSP (she/her)
