| Message ID | 20241107232457.4059785-8-dionnaglaze@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | None | expand |
On 11/7/24 17:24, Dionna Glaze wrote: > sev_issue_cmd_external_user is the only function that checks permissions > before performing its task. With the new GCTX API, it's important to > establish permission once and have that determination dominate later API > uses. This is implicitly how ccp has been used by dominating uses of > sev_do_cmd by a successful sev_issue_cmd_external_user call. > > Consider sev_issue_cmd_external_user deprecated by > checking if a held file descriptor passes file_is_sev, similar to the > file_is_kvm function. > > This also fixes the header comment that the bad file error code is > -%EINVAL when in fact it is -%EBADF. Same comment as before. This commit merely creates a helper function, so this commit message is not appropriate. > > CC: Sean Christopherson <seanjc@google.com> > CC: Paolo Bonzini <pbonzini@redhat.com> > CC: Thomas Gleixner <tglx@linutronix.de> > CC: Ingo Molnar <mingo@redhat.com> > CC: Borislav Petkov <bp@alien8.de> > CC: Dave Hansen <dave.hansen@linux.intel.com> > CC: Ashish Kalra <ashish.kalra@amd.com> > CC: Tom Lendacky <thomas.lendacky@amd.com> > CC: John Allen <john.allen@amd.com> > CC: Herbert Xu <herbert@gondor.apana.org.au> > CC: "David S. Miller" <davem@davemloft.net> > CC: Michael Roth <michael.roth@amd.com> > CC: Luis Chamberlain <mcgrof@kernel.org> > CC: Russ Weight <russ.weight@linux.dev> > CC: Danilo Krummrich <dakr@redhat.com> > CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > CC: "Rafael J. Wysocki" <rafael@kernel.org> > CC: Tianfei zhang <tianfei.zhang@intel.com> > CC: Alexey Kardashevskiy <aik@amd.com> > > Signed-off-by: Dionna Glaze <dionnaglaze@google.com> > --- > drivers/crypto/ccp/sev-dev.c | 13 +++++++++++-- > include/linux/psp-sev.h | 11 ++++++++++- > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index 498ec8a0deeca..f92e6a222da8a 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -8,6 +8,7 @@ > */ > > #include <linux/bitfield.h> > +#include <linux/file.h> > #include <linux/module.h> > #include <linux/kernel.h> > #include <linux/kthread.h> > @@ -2486,11 +2487,19 @@ static struct notifier_block snp_panic_notifier = { > .notifier_call = snp_shutdown_on_panic, > }; > > +bool file_is_sev(struct file *p) > +{ > + return p && p->f_op == &sev_fops; > +} > +EXPORT_SYMBOL_GPL(file_is_sev); > + > int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd, > void *data, int *error) > { > - if (!filep || filep->f_op != &sev_fops) > - return -EBADF; > + int rc = file_is_sev(filep) ? 0 : -EBADF; > + > + if (rc) > + return rc; Get rid of rc and just do: if (!file_is_sev(filep)) return -EBADF; Thanks, Tom > > return sev_do_cmd(cmd, data, error); > } > diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h > index b91cbdc208f49..ed85c0cfcfcbe 100644 > --- a/include/linux/psp-sev.h > +++ b/include/linux/psp-sev.h > @@ -879,11 +879,18 @@ int sev_platform_status(struct sev_user_data_status *status, int *error); > * -%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 > - * -%EINVAL if the SEV file descriptor is not valid > + * -%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 > + * > + * @filep - SEV device file pointer > + */ > +bool file_is_sev(struct file *filep); > + > /** > * sev_guest_deactivate - perform SEV DEACTIVATE command > * > @@ -1039,6 +1046,8 @@ 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); } > > static inline void *snp_alloc_firmware_page(gfp_t mask)
On Mon, Nov 11, 2024 at 2:46 PM Tom Lendacky <thomas.lendacky@amd.com> wrote: > > On 11/7/24 17:24, Dionna Glaze wrote: > > sev_issue_cmd_external_user is the only function that checks permissions > > before performing its task. With the new GCTX API, it's important to > > establish permission once and have that determination dominate later API > > uses. This is implicitly how ccp has been used by dominating uses of > > sev_do_cmd by a successful sev_issue_cmd_external_user call. > > > > Consider sev_issue_cmd_external_user deprecated by > > checking if a held file descriptor passes file_is_sev, similar to the > > file_is_kvm function. > > > > This also fixes the header comment that the bad file error code is > > -%EINVAL when in fact it is -%EBADF. > > Same comment as before. This commit merely creates a helper function, so > this commit message is not appropriate. > Is this a meta-comment about how the commit presupposes being in a series with a goal, but should have a self-contained commit message? I don't know what "same comment as before" you're referring to. How about this: crypto: ccp: Add file_is_sev to identify access Access to the ccp driver only needs to be determined once, so sev_issue_cmd_external_user called in a loop (e.g. for SNP_LAUNCH_UPDATE) does more than it needs to. The file_is_sev function allows the caller to determine access before using sev_do_cmd or other API methods multiple times without extra access checking. This also fixes the header comment that the bad file error code is -%EINVAL when in fact it is -%EBADF.
On 11/12/24 13:47, Dionna Amalie Glaze wrote: > On Mon, Nov 11, 2024 at 2:46 PM Tom Lendacky <thomas.lendacky@amd.com> wrote: >> >> On 11/7/24 17:24, Dionna Glaze wrote: >>> sev_issue_cmd_external_user is the only function that checks permissions >>> before performing its task. With the new GCTX API, it's important to >>> establish permission once and have that determination dominate later API >>> uses. This is implicitly how ccp has been used by dominating uses of >>> sev_do_cmd by a successful sev_issue_cmd_external_user call. >>> >>> Consider sev_issue_cmd_external_user deprecated by >>> checking if a held file descriptor passes file_is_sev, similar to the >>> file_is_kvm function. >>> >>> This also fixes the header comment that the bad file error code is >>> -%EINVAL when in fact it is -%EBADF. >> >> Same comment as before. This commit merely creates a helper function, so >> this commit message is not appropriate. >> > > Is this a meta-comment about how the commit presupposes being in a > series with a goal, but should have a self-contained commit message? I > don't know what "same comment as before" you're referring to. I made the same comment in your previous series. > How about this: > > crypto: ccp: Add file_is_sev to identify access > > Access to the ccp driver only needs to be determined once, so once per KVM ioctl invocation > sev_issue_cmd_external_user called in a loop (e.g. for > SNP_LAUNCH_UPDATE) does more than it needs to. > > The file_is_sev function allows the caller to determine access before using > sev_do_cmd or other API methods multiple times without extra access > checking. > > This also fixes the header comment that the bad file error code is > -%EINVAL when in fact it is -%EBADF. Yes, I like this better. Thanks, Tom > > > >
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index 498ec8a0deeca..f92e6a222da8a 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -8,6 +8,7 @@ */ #include <linux/bitfield.h> +#include <linux/file.h> #include <linux/module.h> #include <linux/kernel.h> #include <linux/kthread.h> @@ -2486,11 +2487,19 @@ static struct notifier_block snp_panic_notifier = { .notifier_call = snp_shutdown_on_panic, }; +bool file_is_sev(struct file *p) +{ + return p && p->f_op == &sev_fops; +} +EXPORT_SYMBOL_GPL(file_is_sev); + int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd, void *data, int *error) { - if (!filep || filep->f_op != &sev_fops) - return -EBADF; + int rc = file_is_sev(filep) ? 0 : -EBADF; + + if (rc) + return rc; return sev_do_cmd(cmd, data, error); } diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h index b91cbdc208f49..ed85c0cfcfcbe 100644 --- a/include/linux/psp-sev.h +++ b/include/linux/psp-sev.h @@ -879,11 +879,18 @@ int sev_platform_status(struct sev_user_data_status *status, int *error); * -%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 - * -%EINVAL if the SEV file descriptor is not valid + * -%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 + * + * @filep - SEV device file pointer + */ +bool file_is_sev(struct file *filep); + /** * sev_guest_deactivate - perform SEV DEACTIVATE command * @@ -1039,6 +1046,8 @@ 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); } static inline void *snp_alloc_firmware_page(gfp_t mask)
sev_issue_cmd_external_user is the only function that checks permissions before performing its task. With the new GCTX API, it's important to establish permission once and have that determination dominate later API uses. This is implicitly how ccp has been used by dominating uses of sev_do_cmd by a successful sev_issue_cmd_external_user call. Consider sev_issue_cmd_external_user deprecated by checking if a held file descriptor passes file_is_sev, similar to the file_is_kvm function. This also fixes the header comment that the bad file error code is -%EINVAL when in fact it is -%EBADF. CC: Sean Christopherson <seanjc@google.com> CC: Paolo Bonzini <pbonzini@redhat.com> CC: Thomas Gleixner <tglx@linutronix.de> CC: Ingo Molnar <mingo@redhat.com> CC: Borislav Petkov <bp@alien8.de> CC: Dave Hansen <dave.hansen@linux.intel.com> CC: Ashish Kalra <ashish.kalra@amd.com> CC: Tom Lendacky <thomas.lendacky@amd.com> CC: John Allen <john.allen@amd.com> CC: Herbert Xu <herbert@gondor.apana.org.au> CC: "David S. Miller" <davem@davemloft.net> CC: Michael Roth <michael.roth@amd.com> CC: Luis Chamberlain <mcgrof@kernel.org> CC: Russ Weight <russ.weight@linux.dev> CC: Danilo Krummrich <dakr@redhat.com> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> CC: "Rafael J. Wysocki" <rafael@kernel.org> CC: Tianfei zhang <tianfei.zhang@intel.com> CC: Alexey Kardashevskiy <aik@amd.com> Signed-off-by: Dionna Glaze <dionnaglaze@google.com> --- drivers/crypto/ccp/sev-dev.c | 13 +++++++++++-- include/linux/psp-sev.h | 11 ++++++++++- 2 files changed, 21 insertions(+), 3 deletions(-)