* Re: [PATCH 1/1] crypto: ccp: use file mode for sev ioctl permissions
2020-03-06 17:20 ` [PATCH 1/1] " Connor Kuehl
@ 2020-03-08 21:54 ` David Rientjes
2020-03-09 14:03 ` Nathaniel McCallum
2020-03-10 14:37 ` Brijesh Singh
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: David Rientjes @ 2020-03-08 21:54 UTC (permalink / raw)
To: Connor Kuehl
Cc: thomas.lendacky, herbert, davem, gary.hook, erdemaktas,
brijesh.singh, npmccallum, bsd, linux-crypto, linux-kernel
On Fri, 6 Mar 2020, Connor Kuehl wrote:
> Instead of using CAP_SYS_ADMIN which is restricted to the root user,
> check the file mode for write permissions before executing commands that
> can affect the platform. This allows for more fine-grained access
> control to the SEV ioctl interface. This would allow a SEV-only user
> or group the ability to administer the platform without requiring them
> to be root or granting them overly powerful permissions.
>
> For example:
>
> chown root:root /dev/sev
> chmod 600 /dev/sev
Hi Connor,
I'm curious why do you need to do the two above commands? It implies that
/dev/sev is either not owned by root or that it is not already restricted
to only being owner read and writable.
Or perhaps these two commands were included only for clarity to explain
what the defaults should be?
> setfacl -m g:sev:r /dev/sev
> setfacl -m g:sev-admin:rw /dev/sev
>
> In this instance, members of the "sev-admin" group have the ability to
> perform all ioctl calls (including the ones that modify platform state).
> Members of the "sev" group only have access to the ioctls that do not
> modify the platform state.
>
> This also makes opening "/dev/sev" more consistent with how file
> descriptors are usually handled. By only checking for CAP_SYS_ADMIN,
> the file descriptor could be opened read-only but could still execute
> ioctls that modify the platform state. This patch enforces that the file
> descriptor is opened with write privileges if it is going to be used to
> modify the platform state.
>
> This flexibility is completely opt-in, and if it is not desirable by
> the administrator then they do not need to give anyone else access to
> /dev/sev.
>
> Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
> ---
> drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++----------------
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index e467860f797d..416b80938a3e 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -283,11 +283,11 @@ static int sev_get_platform_state(int *state, int *error)
> return rc;
> }
>
> -static int sev_ioctl_do_reset(struct sev_issue_cmd *argp)
> +static int sev_ioctl_do_reset(struct sev_issue_cmd *argp, bool writable)
> {
> int state, rc;
>
> - if (!capable(CAP_SYS_ADMIN))
> + if (!writable)
> return -EPERM;
>
> /*
> @@ -331,12 +331,12 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
> return ret;
> }
>
> -static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp)
> +static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool writable)
> {
> struct sev_device *sev = psp_master->sev_data;
> int rc;
>
> - if (!capable(CAP_SYS_ADMIN))
> + if (!writable)
> return -EPERM;
>
> if (sev->state == SEV_STATE_UNINIT) {
> @@ -348,7 +348,7 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp)
> return __sev_do_cmd_locked(cmd, NULL, &argp->error);
> }
>
> -static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp)
> +static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
> {
> struct sev_device *sev = psp_master->sev_data;
> struct sev_user_data_pek_csr input;
> @@ -356,7 +356,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp)
> void *blob = NULL;
> int ret;
>
> - if (!capable(CAP_SYS_ADMIN))
> + if (!writable)
> return -EPERM;
>
> if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
> @@ -539,7 +539,7 @@ static int sev_update_firmware(struct device *dev)
> return ret;
> }
>
> -static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp)
> +static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
> {
> struct sev_device *sev = psp_master->sev_data;
> struct sev_user_data_pek_cert_import input;
> @@ -547,7 +547,7 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp)
> void *pek_blob, *oca_blob;
> int ret;
>
> - if (!capable(CAP_SYS_ADMIN))
> + if (!writable)
> return -EPERM;
>
> if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
> @@ -698,7 +698,7 @@ static int sev_ioctl_do_get_id(struct sev_issue_cmd *argp)
> return ret;
> }
>
> -static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp)
> +static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
> {
> struct sev_device *sev = psp_master->sev_data;
> struct sev_user_data_pdh_cert_export input;
> @@ -708,7 +708,7 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp)
>
> /* If platform is not in INIT state then transition it to INIT. */
> if (sev->state != SEV_STATE_INIT) {
> - if (!capable(CAP_SYS_ADMIN))
> + if (!writable)
> return -EPERM;
>
> ret = __sev_platform_init_locked(&argp->error);
> @@ -801,6 +801,7 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> void __user *argp = (void __user *)arg;
> struct sev_issue_cmd input;
> int ret = -EFAULT;
> + bool writable = file->f_mode & FMODE_WRITE;
>
> if (!psp_master || !psp_master->sev_data)
> return -ENODEV;
> @@ -819,25 +820,25 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> switch (input.cmd) {
>
> case SEV_FACTORY_RESET:
> - ret = sev_ioctl_do_reset(&input);
> + ret = sev_ioctl_do_reset(&input, writable);
> break;
> case SEV_PLATFORM_STATUS:
> ret = sev_ioctl_do_platform_status(&input);
> break;
> case SEV_PEK_GEN:
> - ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PEK_GEN, &input);
> + ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PEK_GEN, &input, writable);
> break;
> case SEV_PDH_GEN:
> - ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PDH_GEN, &input);
> + ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PDH_GEN, &input, writable);
> break;
> case SEV_PEK_CSR:
> - ret = sev_ioctl_do_pek_csr(&input);
> + ret = sev_ioctl_do_pek_csr(&input, writable);
> break;
> case SEV_PEK_CERT_IMPORT:
> - ret = sev_ioctl_do_pek_import(&input);
> + ret = sev_ioctl_do_pek_import(&input, writable);
> break;
> case SEV_PDH_CERT_EXPORT:
> - ret = sev_ioctl_do_pdh_export(&input);
> + ret = sev_ioctl_do_pdh_export(&input, writable);
> break;
> case SEV_GET_ID:
> pr_warn_once("SEV_GET_ID command is deprecated, use SEV_GET_ID2\n");
> --
> 2.24.1
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/1] crypto: ccp: use file mode for sev ioctl permissions
2020-03-08 21:54 ` David Rientjes
@ 2020-03-09 14:03 ` Nathaniel McCallum
2020-03-10 0:43 ` David Rientjes
0 siblings, 1 reply; 11+ messages in thread
From: Nathaniel McCallum @ 2020-03-09 14:03 UTC (permalink / raw)
To: David Rientjes
Cc: Connor Kuehl, Lendacky, Thomas, Herbert Xu, davem, Hook, Gary,
erdemaktas, Singh, Brijesh, Bandan Das, linux-crypto,
linux-kernel
On Sun, Mar 8, 2020 at 5:54 PM David Rientjes <rientjes@google.com> wrote:
>
> On Fri, 6 Mar 2020, Connor Kuehl wrote:
>
> > Instead of using CAP_SYS_ADMIN which is restricted to the root user,
> > check the file mode for write permissions before executing commands that
> > can affect the platform. This allows for more fine-grained access
> > control to the SEV ioctl interface. This would allow a SEV-only user
> > or group the ability to administer the platform without requiring them
> > to be root or granting them overly powerful permissions.
> >
> > For example:
> >
> > chown root:root /dev/sev
> > chmod 600 /dev/sev
>
> Hi Connor,
>
> I'm curious why do you need to do the two above commands? It implies that
> /dev/sev is either not owned by root or that it is not already restricted
> to only being owner read and writable.
>
> Or perhaps these two commands were included only for clarity to explain
> what the defaults should be?
Correct. Those are just exemplary. They represent the existing permissions.
> > setfacl -m g:sev:r /dev/sev
> > setfacl -m g:sev-admin:rw /dev/sev
> >
> > In this instance, members of the "sev-admin" group have the ability to
> > perform all ioctl calls (including the ones that modify platform state).
> > Members of the "sev" group only have access to the ioctls that do not
> > modify the platform state.
> >
> > This also makes opening "/dev/sev" more consistent with how file
> > descriptors are usually handled. By only checking for CAP_SYS_ADMIN,
> > the file descriptor could be opened read-only but could still execute
> > ioctls that modify the platform state. This patch enforces that the file
> > descriptor is opened with write privileges if it is going to be used to
> > modify the platform state.
> >
> > This flexibility is completely opt-in, and if it is not desirable by
> > the administrator then they do not need to give anyone else access to
> > /dev/sev.
> >
> > Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
> > ---
> > drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++----------------
> > 1 file changed, 17 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index e467860f797d..416b80938a3e 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -283,11 +283,11 @@ static int sev_get_platform_state(int *state, int *error)
> > return rc;
> > }
> >
> > -static int sev_ioctl_do_reset(struct sev_issue_cmd *argp)
> > +static int sev_ioctl_do_reset(struct sev_issue_cmd *argp, bool writable)
> > {
> > int state, rc;
> >
> > - if (!capable(CAP_SYS_ADMIN))
> > + if (!writable)
> > return -EPERM;
> >
> > /*
> > @@ -331,12 +331,12 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
> > return ret;
> > }
> >
> > -static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp)
> > +static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool writable)
> > {
> > struct sev_device *sev = psp_master->sev_data;
> > int rc;
> >
> > - if (!capable(CAP_SYS_ADMIN))
> > + if (!writable)
> > return -EPERM;
> >
> > if (sev->state == SEV_STATE_UNINIT) {
> > @@ -348,7 +348,7 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp)
> > return __sev_do_cmd_locked(cmd, NULL, &argp->error);
> > }
> >
> > -static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp)
> > +static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
> > {
> > struct sev_device *sev = psp_master->sev_data;
> > struct sev_user_data_pek_csr input;
> > @@ -356,7 +356,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp)
> > void *blob = NULL;
> > int ret;
> >
> > - if (!capable(CAP_SYS_ADMIN))
> > + if (!writable)
> > return -EPERM;
> >
> > if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
> > @@ -539,7 +539,7 @@ static int sev_update_firmware(struct device *dev)
> > return ret;
> > }
> >
> > -static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp)
> > +static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
> > {
> > struct sev_device *sev = psp_master->sev_data;
> > struct sev_user_data_pek_cert_import input;
> > @@ -547,7 +547,7 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp)
> > void *pek_blob, *oca_blob;
> > int ret;
> >
> > - if (!capable(CAP_SYS_ADMIN))
> > + if (!writable)
> > return -EPERM;
> >
> > if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
> > @@ -698,7 +698,7 @@ static int sev_ioctl_do_get_id(struct sev_issue_cmd *argp)
> > return ret;
> > }
> >
> > -static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp)
> > +static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
> > {
> > struct sev_device *sev = psp_master->sev_data;
> > struct sev_user_data_pdh_cert_export input;
> > @@ -708,7 +708,7 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp)
> >
> > /* If platform is not in INIT state then transition it to INIT. */
> > if (sev->state != SEV_STATE_INIT) {
> > - if (!capable(CAP_SYS_ADMIN))
> > + if (!writable)
> > return -EPERM;
> >
> > ret = __sev_platform_init_locked(&argp->error);
> > @@ -801,6 +801,7 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> > void __user *argp = (void __user *)arg;
> > struct sev_issue_cmd input;
> > int ret = -EFAULT;
> > + bool writable = file->f_mode & FMODE_WRITE;
> >
> > if (!psp_master || !psp_master->sev_data)
> > return -ENODEV;
> > @@ -819,25 +820,25 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> > switch (input.cmd) {
> >
> > case SEV_FACTORY_RESET:
> > - ret = sev_ioctl_do_reset(&input);
> > + ret = sev_ioctl_do_reset(&input, writable);
> > break;
> > case SEV_PLATFORM_STATUS:
> > ret = sev_ioctl_do_platform_status(&input);
> > break;
> > case SEV_PEK_GEN:
> > - ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PEK_GEN, &input);
> > + ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PEK_GEN, &input, writable);
> > break;
> > case SEV_PDH_GEN:
> > - ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PDH_GEN, &input);
> > + ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PDH_GEN, &input, writable);
> > break;
> > case SEV_PEK_CSR:
> > - ret = sev_ioctl_do_pek_csr(&input);
> > + ret = sev_ioctl_do_pek_csr(&input, writable);
> > break;
> > case SEV_PEK_CERT_IMPORT:
> > - ret = sev_ioctl_do_pek_import(&input);
> > + ret = sev_ioctl_do_pek_import(&input, writable);
> > break;
> > case SEV_PDH_CERT_EXPORT:
> > - ret = sev_ioctl_do_pdh_export(&input);
> > + ret = sev_ioctl_do_pdh_export(&input, writable);
> > break;
> > case SEV_GET_ID:
> > pr_warn_once("SEV_GET_ID command is deprecated, use SEV_GET_ID2\n");
> > --
> > 2.24.1
> >
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/1] crypto: ccp: use file mode for sev ioctl permissions
2020-03-09 14:03 ` Nathaniel McCallum
@ 2020-03-10 0:43 ` David Rientjes
0 siblings, 0 replies; 11+ messages in thread
From: David Rientjes @ 2020-03-10 0:43 UTC (permalink / raw)
To: Nathaniel McCallum
Cc: Connor Kuehl, Lendacky, Thomas, Herbert Xu, davem, Hook, Gary,
erdemaktas, Singh, Brijesh, Bandan Das, linux-crypto,
linux-kernel
On Mon, 9 Mar 2020, Nathaniel McCallum wrote:
> > > Instead of using CAP_SYS_ADMIN which is restricted to the root user,
> > > check the file mode for write permissions before executing commands that
> > > can affect the platform. This allows for more fine-grained access
> > > control to the SEV ioctl interface. This would allow a SEV-only user
> > > or group the ability to administer the platform without requiring them
> > > to be root or granting them overly powerful permissions.
> > >
> > > For example:
> > >
> > > chown root:root /dev/sev
> > > chmod 600 /dev/sev
> >
> > Hi Connor,
> >
> > I'm curious why do you need to do the two above commands? It implies that
> > /dev/sev is either not owned by root or that it is not already restricted
> > to only being owner read and writable.
> >
> > Or perhaps these two commands were included only for clarity to explain
> > what the defaults should be?
>
> Correct. Those are just exemplary. They represent the existing permissions.
>
Sounds good, thanks. Maybe showing what ls -l /dev/sev looks like by
default in the changelog would make it clear. Otherwise looks good.
> > > setfacl -m g:sev:r /dev/sev
> > > setfacl -m g:sev-admin:rw /dev/sev
> > >
> > > In this instance, members of the "sev-admin" group have the ability to
> > > perform all ioctl calls (including the ones that modify platform state).
> > > Members of the "sev" group only have access to the ioctls that do not
> > > modify the platform state.
> > >
> > > This also makes opening "/dev/sev" more consistent with how file
> > > descriptors are usually handled. By only checking for CAP_SYS_ADMIN,
> > > the file descriptor could be opened read-only but could still execute
> > > ioctls that modify the platform state. This patch enforces that the file
> > > descriptor is opened with write privileges if it is going to be used to
> > > modify the platform state.
> > >
> > > This flexibility is completely opt-in, and if it is not desirable by
> > > the administrator then they do not need to give anyone else access to
> > > /dev/sev.
> > >
> > > Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
> > > ---
> > > drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++----------------
> > > 1 file changed, 17 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > > index e467860f797d..416b80938a3e 100644
> > > --- a/drivers/crypto/ccp/sev-dev.c
> > > +++ b/drivers/crypto/ccp/sev-dev.c
> > > @@ -283,11 +283,11 @@ static int sev_get_platform_state(int *state, int *error)
> > > return rc;
> > > }
> > >
> > > -static int sev_ioctl_do_reset(struct sev_issue_cmd *argp)
> > > +static int sev_ioctl_do_reset(struct sev_issue_cmd *argp, bool writable)
> > > {
> > > int state, rc;
> > >
> > > - if (!capable(CAP_SYS_ADMIN))
> > > + if (!writable)
> > > return -EPERM;
> > >
> > > /*
> > > @@ -331,12 +331,12 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
> > > return ret;
> > > }
> > >
> > > -static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp)
> > > +static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool writable)
> > > {
> > > struct sev_device *sev = psp_master->sev_data;
> > > int rc;
> > >
> > > - if (!capable(CAP_SYS_ADMIN))
> > > + if (!writable)
> > > return -EPERM;
> > >
> > > if (sev->state == SEV_STATE_UNINIT) {
> > > @@ -348,7 +348,7 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp)
> > > return __sev_do_cmd_locked(cmd, NULL, &argp->error);
> > > }
> > >
> > > -static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp)
> > > +static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
> > > {
> > > struct sev_device *sev = psp_master->sev_data;
> > > struct sev_user_data_pek_csr input;
> > > @@ -356,7 +356,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp)
> > > void *blob = NULL;
> > > int ret;
> > >
> > > - if (!capable(CAP_SYS_ADMIN))
> > > + if (!writable)
> > > return -EPERM;
> > >
> > > if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
> > > @@ -539,7 +539,7 @@ static int sev_update_firmware(struct device *dev)
> > > return ret;
> > > }
> > >
> > > -static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp)
> > > +static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
> > > {
> > > struct sev_device *sev = psp_master->sev_data;
> > > struct sev_user_data_pek_cert_import input;
> > > @@ -547,7 +547,7 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp)
> > > void *pek_blob, *oca_blob;
> > > int ret;
> > >
> > > - if (!capable(CAP_SYS_ADMIN))
> > > + if (!writable)
> > > return -EPERM;
> > >
> > > if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
> > > @@ -698,7 +698,7 @@ static int sev_ioctl_do_get_id(struct sev_issue_cmd *argp)
> > > return ret;
> > > }
> > >
> > > -static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp)
> > > +static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
> > > {
> > > struct sev_device *sev = psp_master->sev_data;
> > > struct sev_user_data_pdh_cert_export input;
> > > @@ -708,7 +708,7 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp)
> > >
> > > /* If platform is not in INIT state then transition it to INIT. */
> > > if (sev->state != SEV_STATE_INIT) {
> > > - if (!capable(CAP_SYS_ADMIN))
> > > + if (!writable)
> > > return -EPERM;
> > >
> > > ret = __sev_platform_init_locked(&argp->error);
> > > @@ -801,6 +801,7 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> > > void __user *argp = (void __user *)arg;
> > > struct sev_issue_cmd input;
> > > int ret = -EFAULT;
> > > + bool writable = file->f_mode & FMODE_WRITE;
> > >
> > > if (!psp_master || !psp_master->sev_data)
> > > return -ENODEV;
> > > @@ -819,25 +820,25 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> > > switch (input.cmd) {
> > >
> > > case SEV_FACTORY_RESET:
> > > - ret = sev_ioctl_do_reset(&input);
> > > + ret = sev_ioctl_do_reset(&input, writable);
> > > break;
> > > case SEV_PLATFORM_STATUS:
> > > ret = sev_ioctl_do_platform_status(&input);
> > > break;
> > > case SEV_PEK_GEN:
> > > - ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PEK_GEN, &input);
> > > + ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PEK_GEN, &input, writable);
> > > break;
> > > case SEV_PDH_GEN:
> > > - ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PDH_GEN, &input);
> > > + ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PDH_GEN, &input, writable);
> > > break;
> > > case SEV_PEK_CSR:
> > > - ret = sev_ioctl_do_pek_csr(&input);
> > > + ret = sev_ioctl_do_pek_csr(&input, writable);
> > > break;
> > > case SEV_PEK_CERT_IMPORT:
> > > - ret = sev_ioctl_do_pek_import(&input);
> > > + ret = sev_ioctl_do_pek_import(&input, writable);
> > > break;
> > > case SEV_PDH_CERT_EXPORT:
> > > - ret = sev_ioctl_do_pdh_export(&input);
> > > + ret = sev_ioctl_do_pdh_export(&input, writable);
> > > break;
> > > case SEV_GET_ID:
> > > pr_warn_once("SEV_GET_ID command is deprecated, use SEV_GET_ID2\n");
> > > --
> > > 2.24.1
> > >
> > >
> >
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] crypto: ccp: use file mode for sev ioctl permissions
2020-03-06 17:20 ` [PATCH 1/1] " Connor Kuehl
2020-03-08 21:54 ` David Rientjes
@ 2020-03-10 14:37 ` Brijesh Singh
2020-03-10 14:45 ` Bandan Das
2020-03-10 19:53 ` Tom Lendacky
2020-03-12 12:40 ` Herbert Xu
3 siblings, 1 reply; 11+ messages in thread
From: Brijesh Singh @ 2020-03-10 14:37 UTC (permalink / raw)
To: Connor Kuehl, thomas.lendacky, herbert, davem
Cc: brijesh.singh, gary.hook, erdemaktas, rientjes, npmccallum, bsd,
linux-crypto, linux-kernel
On 3/6/20 11:20 AM, Connor Kuehl wrote:
> Instead of using CAP_SYS_ADMIN which is restricted to the root user,
> check the file mode for write permissions before executing commands that
> can affect the platform. This allows for more fine-grained access
> control to the SEV ioctl interface. This would allow a SEV-only user
> or group the ability to administer the platform without requiring them
> to be root or granting them overly powerful permissions.
>
> For example:
>
> chown root:root /dev/sev
> chmod 600 /dev/sev
> setfacl -m g:sev:r /dev/sev
> setfacl -m g:sev-admin:rw /dev/sev
>
> In this instance, members of the "sev-admin" group have the ability to
> perform all ioctl calls (including the ones that modify platform state).
> Members of the "sev" group only have access to the ioctls that do not
> modify the platform state.
>
> This also makes opening "/dev/sev" more consistent with how file
> descriptors are usually handled. By only checking for CAP_SYS_ADMIN,
> the file descriptor could be opened read-only but could still execute
> ioctls that modify the platform state. This patch enforces that the file
> descriptor is opened with write privileges if it is going to be used to
> modify the platform state.
>
> This flexibility is completely opt-in, and if it is not desirable by
> the administrator then they do not need to give anyone else access to
> /dev/sev.
>
> Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
> ---
> drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++----------------
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>
thanks
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] crypto: ccp: use file mode for sev ioctl permissions
2020-03-10 14:37 ` Brijesh Singh
@ 2020-03-10 14:45 ` Bandan Das
2020-03-10 19:02 ` David Rientjes
0 siblings, 1 reply; 11+ messages in thread
From: Bandan Das @ 2020-03-10 14:45 UTC (permalink / raw)
To: Brijesh Singh
Cc: Connor Kuehl, thomas.lendacky, herbert, davem, gary.hook,
erdemaktas, rientjes, npmccallum, linux-crypto, linux-kernel
Brijesh Singh <brijesh.singh@amd.com> writes:
> On 3/6/20 11:20 AM, Connor Kuehl wrote:
>> Instead of using CAP_SYS_ADMIN which is restricted to the root user,
>> check the file mode for write permissions before executing commands that
>> can affect the platform. This allows for more fine-grained access
>> control to the SEV ioctl interface. This would allow a SEV-only user
>> or group the ability to administer the platform without requiring them
>> to be root or granting them overly powerful permissions.
>>
>> For example:
>>
>> chown root:root /dev/sev
>> chmod 600 /dev/sev
>> setfacl -m g:sev:r /dev/sev
>> setfacl -m g:sev-admin:rw /dev/sev
>>
>> In this instance, members of the "sev-admin" group have the ability to
>> perform all ioctl calls (including the ones that modify platform state).
>> Members of the "sev" group only have access to the ioctls that do not
>> modify the platform state.
>>
>> This also makes opening "/dev/sev" more consistent with how file
>> descriptors are usually handled. By only checking for CAP_SYS_ADMIN,
>> the file descriptor could be opened read-only but could still execute
>> ioctls that modify the platform state. This patch enforces that the file
>> descriptor is opened with write privileges if it is going to be used to
>> modify the platform state.
>>
>> This flexibility is completely opt-in, and if it is not desirable by
>> the administrator then they do not need to give anyone else access to
>> /dev/sev.
>>
>> Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
>> ---
>> drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++----------------
>> 1 file changed, 17 insertions(+), 16 deletions(-)
>>
>
> Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>
>
> thanks
Reviewed-by: Bandan Das <bsd@redhat.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] crypto: ccp: use file mode for sev ioctl permissions
2020-03-10 14:45 ` Bandan Das
@ 2020-03-10 19:02 ` David Rientjes
0 siblings, 0 replies; 11+ messages in thread
From: David Rientjes @ 2020-03-10 19:02 UTC (permalink / raw)
To: Bandan Das
Cc: Brijesh Singh, Connor Kuehl, thomas.lendacky, herbert, davem,
gary.hook, erdemaktas, npmccallum, linux-crypto, linux-kernel
On Tue, 10 Mar 2020, Bandan Das wrote:
> Brijesh Singh <brijesh.singh@amd.com> writes:
>
> > On 3/6/20 11:20 AM, Connor Kuehl wrote:
> >> Instead of using CAP_SYS_ADMIN which is restricted to the root user,
> >> check the file mode for write permissions before executing commands that
> >> can affect the platform. This allows for more fine-grained access
> >> control to the SEV ioctl interface. This would allow a SEV-only user
> >> or group the ability to administer the platform without requiring them
> >> to be root or granting them overly powerful permissions.
> >>
> >> For example:
> >>
> >> chown root:root /dev/sev
> >> chmod 600 /dev/sev
> >> setfacl -m g:sev:r /dev/sev
> >> setfacl -m g:sev-admin:rw /dev/sev
> >>
> >> In this instance, members of the "sev-admin" group have the ability to
> >> perform all ioctl calls (including the ones that modify platform state).
> >> Members of the "sev" group only have access to the ioctls that do not
> >> modify the platform state.
> >>
> >> This also makes opening "/dev/sev" more consistent with how file
> >> descriptors are usually handled. By only checking for CAP_SYS_ADMIN,
> >> the file descriptor could be opened read-only but could still execute
> >> ioctls that modify the platform state. This patch enforces that the file
> >> descriptor is opened with write privileges if it is going to be used to
> >> modify the platform state.
> >>
> >> This flexibility is completely opt-in, and if it is not desirable by
> >> the administrator then they do not need to give anyone else access to
> >> /dev/sev.
> >>
> >> Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
> >> ---
> >> drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++----------------
> >> 1 file changed, 17 insertions(+), 16 deletions(-)
> >>
> >
> > Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>
> >
> > thanks
>
> Reviewed-by: Bandan Das <bsd@redhat.com>
>
Acked-by: David Rientjes <rientjes@google.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] crypto: ccp: use file mode for sev ioctl permissions
2020-03-06 17:20 ` [PATCH 1/1] " Connor Kuehl
2020-03-08 21:54 ` David Rientjes
2020-03-10 14:37 ` Brijesh Singh
@ 2020-03-10 19:53 ` Tom Lendacky
2020-03-12 12:40 ` Herbert Xu
3 siblings, 0 replies; 11+ messages in thread
From: Tom Lendacky @ 2020-03-10 19:53 UTC (permalink / raw)
To: Connor Kuehl, herbert, davem
Cc: gary.hook, erdemaktas, rientjes, brijesh.singh, npmccallum, bsd,
linux-crypto, linux-kernel
On 3/6/20 11:20 AM, Connor Kuehl wrote:
> Instead of using CAP_SYS_ADMIN which is restricted to the root user,
> check the file mode for write permissions before executing commands that
> can affect the platform. This allows for more fine-grained access
> control to the SEV ioctl interface. This would allow a SEV-only user
> or group the ability to administer the platform without requiring them
> to be root or granting them overly powerful permissions.
>
> For example:
>
> chown root:root /dev/sev
> chmod 600 /dev/sev
> setfacl -m g:sev:r /dev/sev
> setfacl -m g:sev-admin:rw /dev/sev
>
> In this instance, members of the "sev-admin" group have the ability to
> perform all ioctl calls (including the ones that modify platform state).
> Members of the "sev" group only have access to the ioctls that do not
> modify the platform state.
>
> This also makes opening "/dev/sev" more consistent with how file
> descriptors are usually handled. By only checking for CAP_SYS_ADMIN,
> the file descriptor could be opened read-only but could still execute
> ioctls that modify the platform state. This patch enforces that the file
> descriptor is opened with write privileges if it is going to be used to
> modify the platform state.
>
> This flexibility is completely opt-in, and if it is not desirable by
> the administrator then they do not need to give anyone else access to
> /dev/sev.
>
> Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
Acked-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++----------------
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index e467860f797d..416b80938a3e 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -283,11 +283,11 @@ static int sev_get_platform_state(int *state, int *error)
> return rc;
> }
>
> -static int sev_ioctl_do_reset(struct sev_issue_cmd *argp)
> +static int sev_ioctl_do_reset(struct sev_issue_cmd *argp, bool writable)
> {
> int state, rc;
>
> - if (!capable(CAP_SYS_ADMIN))
> + if (!writable)
> return -EPERM;
>
> /*
> @@ -331,12 +331,12 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
> return ret;
> }
>
> -static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp)
> +static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool writable)
> {
> struct sev_device *sev = psp_master->sev_data;
> int rc;
>
> - if (!capable(CAP_SYS_ADMIN))
> + if (!writable)
> return -EPERM;
>
> if (sev->state == SEV_STATE_UNINIT) {
> @@ -348,7 +348,7 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp)
> return __sev_do_cmd_locked(cmd, NULL, &argp->error);
> }
>
> -static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp)
> +static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
> {
> struct sev_device *sev = psp_master->sev_data;
> struct sev_user_data_pek_csr input;
> @@ -356,7 +356,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp)
> void *blob = NULL;
> int ret;
>
> - if (!capable(CAP_SYS_ADMIN))
> + if (!writable)
> return -EPERM;
>
> if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
> @@ -539,7 +539,7 @@ static int sev_update_firmware(struct device *dev)
> return ret;
> }
>
> -static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp)
> +static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
> {
> struct sev_device *sev = psp_master->sev_data;
> struct sev_user_data_pek_cert_import input;
> @@ -547,7 +547,7 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp)
> void *pek_blob, *oca_blob;
> int ret;
>
> - if (!capable(CAP_SYS_ADMIN))
> + if (!writable)
> return -EPERM;
>
> if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
> @@ -698,7 +698,7 @@ static int sev_ioctl_do_get_id(struct sev_issue_cmd *argp)
> return ret;
> }
>
> -static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp)
> +static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
> {
> struct sev_device *sev = psp_master->sev_data;
> struct sev_user_data_pdh_cert_export input;
> @@ -708,7 +708,7 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp)
>
> /* If platform is not in INIT state then transition it to INIT. */
> if (sev->state != SEV_STATE_INIT) {
> - if (!capable(CAP_SYS_ADMIN))
> + if (!writable)
> return -EPERM;
>
> ret = __sev_platform_init_locked(&argp->error);
> @@ -801,6 +801,7 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> void __user *argp = (void __user *)arg;
> struct sev_issue_cmd input;
> int ret = -EFAULT;
> + bool writable = file->f_mode & FMODE_WRITE;
>
> if (!psp_master || !psp_master->sev_data)
> return -ENODEV;
> @@ -819,25 +820,25 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> switch (input.cmd) {
>
> case SEV_FACTORY_RESET:
> - ret = sev_ioctl_do_reset(&input);
> + ret = sev_ioctl_do_reset(&input, writable);
> break;
> case SEV_PLATFORM_STATUS:
> ret = sev_ioctl_do_platform_status(&input);
> break;
> case SEV_PEK_GEN:
> - ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PEK_GEN, &input);
> + ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PEK_GEN, &input, writable);
> break;
> case SEV_PDH_GEN:
> - ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PDH_GEN, &input);
> + ret = sev_ioctl_do_pek_pdh_gen(SEV_CMD_PDH_GEN, &input, writable);
> break;
> case SEV_PEK_CSR:
> - ret = sev_ioctl_do_pek_csr(&input);
> + ret = sev_ioctl_do_pek_csr(&input, writable);
> break;
> case SEV_PEK_CERT_IMPORT:
> - ret = sev_ioctl_do_pek_import(&input);
> + ret = sev_ioctl_do_pek_import(&input, writable);
> break;
> case SEV_PDH_CERT_EXPORT:
> - ret = sev_ioctl_do_pdh_export(&input);
> + ret = sev_ioctl_do_pdh_export(&input, writable);
> break;
> case SEV_GET_ID:
> pr_warn_once("SEV_GET_ID command is deprecated, use SEV_GET_ID2\n");
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/1] crypto: ccp: use file mode for sev ioctl permissions
2020-03-06 17:20 ` [PATCH 1/1] " Connor Kuehl
` (2 preceding siblings ...)
2020-03-10 19:53 ` Tom Lendacky
@ 2020-03-12 12:40 ` Herbert Xu
3 siblings, 0 replies; 11+ messages in thread
From: Herbert Xu @ 2020-03-12 12:40 UTC (permalink / raw)
To: Connor Kuehl
Cc: thomas.lendacky, davem, gary.hook, erdemaktas, rientjes,
brijesh.singh, npmccallum, bsd, linux-crypto, linux-kernel
On Fri, Mar 06, 2020 at 09:20:10AM -0800, Connor Kuehl wrote:
> Instead of using CAP_SYS_ADMIN which is restricted to the root user,
> check the file mode for write permissions before executing commands that
> can affect the platform. This allows for more fine-grained access
> control to the SEV ioctl interface. This would allow a SEV-only user
> or group the ability to administer the platform without requiring them
> to be root or granting them overly powerful permissions.
>
> For example:
>
> chown root:root /dev/sev
> chmod 600 /dev/sev
> setfacl -m g:sev:r /dev/sev
> setfacl -m g:sev-admin:rw /dev/sev
>
> In this instance, members of the "sev-admin" group have the ability to
> perform all ioctl calls (including the ones that modify platform state).
> Members of the "sev" group only have access to the ioctls that do not
> modify the platform state.
>
> This also makes opening "/dev/sev" more consistent with how file
> descriptors are usually handled. By only checking for CAP_SYS_ADMIN,
> the file descriptor could be opened read-only but could still execute
> ioctls that modify the platform state. This patch enforces that the file
> descriptor is opened with write privileges if it is going to be used to
> modify the platform state.
>
> This flexibility is completely opt-in, and if it is not desirable by
> the administrator then they do not need to give anyone else access to
> /dev/sev.
>
> Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
> ---
> drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++----------------
> 1 file changed, 17 insertions(+), 16 deletions(-)
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 11+ messages in thread