* [PATCH] cxl/mbox: Do not allow immediate mode in SET_PARTITION_INFO
@ 2022-01-03 20:21 alison.schofield
2022-01-04 19:02 ` Dan Williams
2022-01-04 23:21 ` Ben Widawsky
0 siblings, 2 replies; 6+ messages in thread
From: alison.schofield @ 2022-01-03 20:21 UTC (permalink / raw)
To: Ben Widawsky, Dan Williams, Ira Weiny, Vishal Verma
Cc: Alison Schofield, linux-cxl
From: Alison Schofield <alison.schofield@intel.com>
User space may send the SET_PARTITION_INFO mailbox command using
the IOCTL interface. Inspect the input payload and fail if the
immediate flag is set.
This is the first instance of the driver inspecting an input payload
from user space. Assume there will be more such cases and implement
with an extensible helper.
Note: At this time immediate partitioning is not allowed because the
kernel will need to react immediately to this configuration change
and that support is not yet implemented.
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
drivers/cxl/core/mbox.c | 43 +++++++++++++++++++++++++++++++++++++++++
drivers/cxl/cxlmem.h | 7 +++++++
2 files changed, 50 insertions(+)
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index be61a0d8016b..2cf5ccdea7df 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -352,6 +352,41 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd,
return 0;
}
+/**
+ * cxl_payload_from_user_allowed() - Check contents of in_payload.
+ * @opcode: The mailbox command opcode.
+ * @payload_in: Pointer to the input payload passed in from user space.
+ *
+ * Return:
+ * * true - payload_in passes check for @opcode.
+ * * false - payload_in contains invalid or unsupported values.
+ *
+ * The driver may inspect payload contents before sending a mailbox
+ * command from user space to the device. The intent is to reject
+ * commands with input payloads that are known to be unsafe. This
+ * check is not intended to replace the users careful selection of
+ * mailbox command parameters and makes no guarantee that the user
+ * command will succeed, nor that it is appropriate.
+ *
+ * The specific checks are determined by the opcode.
+ */
+static bool cxl_payload_from_user_allowed(u16 opcode, void *payload_in)
+{
+ switch (opcode) {
+ case CXL_MBOX_OP_SET_PARTITION_INFO: {
+ struct cxl_mbox_set_partition_info *pi;
+
+ pi = (struct cxl_mbox_set_partition_info *)payload_in;
+ if (pi->flags && CXL_SET_PARTITION_IMMEDIATE_FLAG)
+ return false;
+ break;
+ }
+ default:
+ break;
+ }
+ return true;
+}
+
/**
* handle_mailbox_cmd_from_user() - Dispatch a mailbox command for userspace.
* @cxlds: The device data for the operation
@@ -405,6 +440,14 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
}
}
+ if (!cxl_payload_from_user_allowed(mbox_cmd.opcode,
+ mbox_cmd.payload_in)) {
+ dev_dbg(dev, "%s: input payload not allowed\n",
+ cxl_command_names[cmd->info.id].name);
+ rc = -EINVAL;
+ goto out;
+ }
+
dev_dbg(dev,
"Submitting %s command for user\n"
"\topcode: %x\n"
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 8d96d009ad90..e10b86f06c75 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -231,6 +231,13 @@ struct cxl_mbox_set_lsa {
u8 data[];
} __packed;
+struct cxl_mbox_set_partition_info {
+ u64 volatile_capacity;
+ u8 flags;
+} __packed;
+
+#define CXL_SET_PARTITION_IMMEDIATE_FLAG BIT(0)
+
/**
* struct cxl_mem_command - Driver representation of a memory device command
* @info: Command information as it exists for the UAPI
base-commit: 53989fad1286e652ea3655ae3367ba698da8d2ff
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] cxl/mbox: Do not allow immediate mode in SET_PARTITION_INFO
2022-01-03 20:21 [PATCH] cxl/mbox: Do not allow immediate mode in SET_PARTITION_INFO alison.schofield
@ 2022-01-04 19:02 ` Dan Williams
2022-01-04 23:11 ` Alison Schofield
2022-01-04 23:21 ` Ben Widawsky
1 sibling, 1 reply; 6+ messages in thread
From: Dan Williams @ 2022-01-04 19:02 UTC (permalink / raw)
To: Schofield, Alison; +Cc: Ben Widawsky, Ira Weiny, Vishal Verma, linux-cxl
On Mon, Jan 3, 2022 at 12:15 PM <alison.schofield@intel.com> wrote:
>
> From: Alison Schofield <alison.schofield@intel.com>
>
> User space may send the SET_PARTITION_INFO mailbox command using
> the IOCTL interface. Inspect the input payload and fail if the
> immediate flag is set.
>
> This is the first instance of the driver inspecting an input payload
> from user space. Assume there will be more such cases and implement
> with an extensible helper.
>
> Note: At this time immediate partitioning is not allowed because the
> kernel will need to react immediately to this configuration change
> and that support is not yet implemented.
I would say:
In order for the kernel to react to an immediate partition change it
needs to assert that the change will not affect any active decode. At
a minimum this requires validating that the device is using HDM
decoders instead of the CXL DVSEC for decode, and that none of the
active HDM decoders are affected by the partition change. For now,
just fail until that support arrives.
>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
> drivers/cxl/core/mbox.c | 43 +++++++++++++++++++++++++++++++++++++++++
> drivers/cxl/cxlmem.h | 7 +++++++
> 2 files changed, 50 insertions(+)
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index be61a0d8016b..2cf5ccdea7df 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -352,6 +352,41 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd,
> return 0;
> }
>
> +/**
> + * cxl_payload_from_user_allowed() - Check contents of in_payload.
> + * @opcode: The mailbox command opcode.
> + * @payload_in: Pointer to the input payload passed in from user space.
> + *
> + * Return:
> + * * true - payload_in passes check for @opcode.
> + * * false - payload_in contains invalid or unsupported values.
> + *
> + * The driver may inspect payload contents before sending a mailbox
> + * command from user space to the device. The intent is to reject
> + * commands with input payloads that are known to be unsafe. This
> + * check is not intended to replace the users careful selection of
> + * mailbox command parameters and makes no guarantee that the user
> + * command will succeed, nor that it is appropriate.
> + *
> + * The specific checks are determined by the opcode.
> + */
> +static bool cxl_payload_from_user_allowed(u16 opcode, void *payload_in)
> +{
> + switch (opcode) {
> + case CXL_MBOX_OP_SET_PARTITION_INFO: {
> + struct cxl_mbox_set_partition_info *pi;
> +
> + pi = (struct cxl_mbox_set_partition_info *)payload_in;
No need for a cast since @payload_in is a 'void *'
> + if (pi->flags && CXL_SET_PARTITION_IMMEDIATE_FLAG)
> + return false;
> + break;
> + }
> + default:
> + break;
> + }
> + return true;
> +}
> +
> /**
> * handle_mailbox_cmd_from_user() - Dispatch a mailbox command for userspace.
> * @cxlds: The device data for the operation
> @@ -405,6 +440,14 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
> }
> }
>
> + if (!cxl_payload_from_user_allowed(mbox_cmd.opcode,
> + mbox_cmd.payload_in)) {
> + dev_dbg(dev, "%s: input payload not allowed\n",
> + cxl_command_names[cmd->info.id].name);
> + rc = -EINVAL;
> + goto out;
> + }
> +
> dev_dbg(dev,
> "Submitting %s command for user\n"
> "\topcode: %x\n"
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 8d96d009ad90..e10b86f06c75 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -231,6 +231,13 @@ struct cxl_mbox_set_lsa {
> u8 data[];
> } __packed;
>
> +struct cxl_mbox_set_partition_info {
> + u64 volatile_capacity;
This should be __le64, and it looks like 'struct cxl_mbox_get_lsa' and
'struct cxl_mbox_set_lsa' need similar fixups.
> + u8 flags;
> +} __packed;
> +
> +#define CXL_SET_PARTITION_IMMEDIATE_FLAG BIT(0)
> +
> /**
> * struct cxl_mem_command - Driver representation of a memory device command
> * @info: Command information as it exists for the UAPI
>
> base-commit: 53989fad1286e652ea3655ae3367ba698da8d2ff
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] cxl/mbox: Do not allow immediate mode in SET_PARTITION_INFO
2022-01-04 19:02 ` Dan Williams
@ 2022-01-04 23:11 ` Alison Schofield
0 siblings, 0 replies; 6+ messages in thread
From: Alison Schofield @ 2022-01-04 23:11 UTC (permalink / raw)
To: Dan Williams; +Cc: Ben Widawsky, Ira Weiny, Vishal Verma, linux-cxl
Thanks for the review...
On Tue, Jan 04, 2022 at 11:02:19AM -0800, Dan Williams wrote:
> On Mon, Jan 3, 2022 at 12:15 PM <alison.schofield@intel.com> wrote:
> >
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > User space may send the SET_PARTITION_INFO mailbox command using
> > the IOCTL interface. Inspect the input payload and fail if the
> > immediate flag is set.
> >
> > This is the first instance of the driver inspecting an input payload
> > from user space. Assume there will be more such cases and implement
> > with an extensible helper.
> >
> > Note: At this time immediate partitioning is not allowed because the
> > kernel will need to react immediately to this configuration change
> > and that support is not yet implemented.
>
> I would say:
>
> In order for the kernel to react to an immediate partition change it
> needs to assert that the change will not affect any active decode. At
> a minimum this requires validating that the device is using HDM
> decoders instead of the CXL DVSEC for decode, and that none of the
> active HDM decoders are affected by the partition change. For now,
> just fail until that support arrives.
>
Got it!
> >
snip
> > +static bool cxl_payload_from_user_allowed(u16 opcode, void *payload_in)
> > +{
> > + switch (opcode) {
> > + case CXL_MBOX_OP_SET_PARTITION_INFO: {
> > + struct cxl_mbox_set_partition_info *pi;
> > +
> > + pi = (struct cxl_mbox_set_partition_info *)payload_in;
>
> No need for a cast since @payload_in is a 'void *'
>
Got it!
> > + if (pi->flags && CXL_SET_PARTITION_IMMEDIATE_FLAG)
> > + return false;
> > + break;
> > + }
> > + default:
> > + break;
> > + }
> > + return true;
> > +}
> > +
snip
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index 8d96d009ad90..e10b86f06c75 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> > @@ -231,6 +231,13 @@ struct cxl_mbox_set_lsa {
> > u8 data[];
> > } __packed;
> >
> > +struct cxl_mbox_set_partition_info {
> > + u64 volatile_capacity;
>
> This should be __le64, and it looks like 'struct cxl_mbox_get_lsa' and
> 'struct cxl_mbox_set_lsa' need similar fixups.
>
Got it and will do!
> > + u8 flags;
> > +} __packed;
> > +
> > +#define CXL_SET_PARTITION_IMMEDIATE_FLAG BIT(0)
> > +
> > /**
> > * struct cxl_mem_command - Driver representation of a memory device command
> > * @info: Command information as it exists for the UAPI
> >
> > base-commit: 53989fad1286e652ea3655ae3367ba698da8d2ff
> > --
> > 2.31.1
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cxl/mbox: Do not allow immediate mode in SET_PARTITION_INFO
2022-01-03 20:21 [PATCH] cxl/mbox: Do not allow immediate mode in SET_PARTITION_INFO alison.schofield
2022-01-04 19:02 ` Dan Williams
@ 2022-01-04 23:21 ` Ben Widawsky
2022-01-05 0:59 ` Alison Schofield
1 sibling, 1 reply; 6+ messages in thread
From: Ben Widawsky @ 2022-01-04 23:21 UTC (permalink / raw)
To: alison.schofield; +Cc: Dan Williams, Ira Weiny, Vishal Verma, linux-cxl
On 22-01-03 12:21:00, alison.schofield@intel.com wrote:
> From: Alison Schofield <alison.schofield@intel.com>
>
> User space may send the SET_PARTITION_INFO mailbox command using
> the IOCTL interface. Inspect the input payload and fail if the
> immediate flag is set.
>
> This is the first instance of the driver inspecting an input payload
> from user space. Assume there will be more such cases and implement
> with an extensible helper.
Not sure if it's useful, but this was implemented at some point:
https://lore.kernel.org/linux-cxl/20210210000259.635748-8-ben.widawsky@intel.com/
>
> Note: At this time immediate partitioning is not allowed because the
> kernel will need to react immediately to this configuration change
> and that support is not yet implemented.
>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
> drivers/cxl/core/mbox.c | 43 +++++++++++++++++++++++++++++++++++++++++
> drivers/cxl/cxlmem.h | 7 +++++++
> 2 files changed, 50 insertions(+)
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index be61a0d8016b..2cf5ccdea7df 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -352,6 +352,41 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd,
> return 0;
> }
>
> +/**
> + * cxl_payload_from_user_allowed() - Check contents of in_payload.
> + * @opcode: The mailbox command opcode.
> + * @payload_in: Pointer to the input payload passed in from user space.
> + *
> + * Return:
> + * * true - payload_in passes check for @opcode.
> + * * false - payload_in contains invalid or unsupported values.
> + *
> + * The driver may inspect payload contents before sending a mailbox
> + * command from user space to the device. The intent is to reject
> + * commands with input payloads that are known to be unsafe. This
> + * check is not intended to replace the users careful selection of
> + * mailbox command parameters and makes no guarantee that the user
> + * command will succeed, nor that it is appropriate.
> + *
> + * The specific checks are determined by the opcode.
> + */
> +static bool cxl_payload_from_user_allowed(u16 opcode, void *payload_in)
> +{
> + switch (opcode) {
> + case CXL_MBOX_OP_SET_PARTITION_INFO: {
> + struct cxl_mbox_set_partition_info *pi;
> +
> + pi = (struct cxl_mbox_set_partition_info *)payload_in;
> + if (pi->flags && CXL_SET_PARTITION_IMMEDIATE_FLAG)
> + return false;
> + break;
> + }
> + default:
> + break;
> + }
> + return true;
> +}
> +
> /**
> * handle_mailbox_cmd_from_user() - Dispatch a mailbox command for userspace.
> * @cxlds: The device data for the operation
> @@ -405,6 +440,14 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
> }
> }
>
> + if (!cxl_payload_from_user_allowed(mbox_cmd.opcode,
> + mbox_cmd.payload_in)) {
> + dev_dbg(dev, "%s: input payload not allowed\n",
> + cxl_command_names[cmd->info.id].name);
> + rc = -EINVAL;
> + goto out;
> + }
> +
Perhaps foolishly, the kdocs for handle_mailbox_cmd_from_user() documents the
error conditions. Would you mind adding EINVAL?
Also, cxl_validate_cmd_from_user() was supposed to handle this kind of stuff.
All validation from user commands should spawn from that. Is there some reason
this one is different?
> dev_dbg(dev,
> "Submitting %s command for user\n"
> "\topcode: %x\n"
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 8d96d009ad90..e10b86f06c75 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -231,6 +231,13 @@ struct cxl_mbox_set_lsa {
> u8 data[];
> } __packed;
>
> +struct cxl_mbox_set_partition_info {
> + u64 volatile_capacity;
> + u8 flags;
> +} __packed;
> +
> +#define CXL_SET_PARTITION_IMMEDIATE_FLAG BIT(0)
> +
I think these defines belong in cxl.h
> /**
> * struct cxl_mem_command - Driver representation of a memory device command
> * @info: Command information as it exists for the UAPI
>
> base-commit: 53989fad1286e652ea3655ae3367ba698da8d2ff
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] cxl/mbox: Do not allow immediate mode in SET_PARTITION_INFO
2022-01-04 23:21 ` Ben Widawsky
@ 2022-01-05 0:59 ` Alison Schofield
2022-01-05 3:22 ` Dan Williams
0 siblings, 1 reply; 6+ messages in thread
From: Alison Schofield @ 2022-01-05 0:59 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Dan Williams, Ira Weiny, Vishal Verma, linux-cxl
Thanks for the review Ben...
On Tue, Jan 04, 2022 at 03:21:34PM -0800, Ben Widawsky wrote:
> On 22-01-03 12:21:00, alison.schofield@intel.com wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > User space may send the SET_PARTITION_INFO mailbox command using
> > the IOCTL interface. Inspect the input payload and fail if the
> > immediate flag is set.
> >
> > This is the first instance of the driver inspecting an input payload
> > from user space. Assume there will be more such cases and implement
> > with an extensible helper.
>
> Not sure if it's useful, but this was implemented at some point:
> https://lore.kernel.org/linux-cxl/20210210000259.635748-8-ben.widawsky@intel.com/
Thanks. Looking.
>
snip
> > @@ -405,6 +440,14 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
> > }
> > }
> >
> > + if (!cxl_payload_from_user_allowed(mbox_cmd.opcode,
> > + mbox_cmd.payload_in)) {
> > + dev_dbg(dev, "%s: input payload not allowed\n",
> > + cxl_command_names[cmd->info.id].name);
> > + rc = -EINVAL;
> > + goto out;
> > + }
> > +
>
> Perhaps foolishly, the kdocs for handle_mailbox_cmd_from_user() documents the
> error conditions. Would you mind adding EINVAL?
I'm going to try to mv the allowable check, so this will go away.
>
> Also, cxl_validate_cmd_from_user() was supposed to handle this kind of stuff.
> All validation from user commands should spawn from that. Is there some reason
> this one is different?
The existing cxl_validate_cmd_from_user() happens before we copy the
payload from user - so this payload check didn't fit in there.
Let me look at reorganizing a bit so validating the cmd and it's payload
can both spawn from cxl_validate_cmd_from_user().
>
snip
>
> > +#define CXL_SET_PARTITION_IMMEDIATE_FLAG BIT(0)
> > +
>
> I think these defines belong in cxl.h
>
It seems this is OK in cxl.h where other CXL spec defines now live.
> > /**
> > * struct cxl_mem_command - Driver representation of a memory device command
> > * @info: Command information as it exists for the UAPI
> >
> > base-commit: 53989fad1286e652ea3655ae3367ba698da8d2ff
> > --
> > 2.31.1
> >
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] cxl/mbox: Do not allow immediate mode in SET_PARTITION_INFO
2022-01-05 0:59 ` Alison Schofield
@ 2022-01-05 3:22 ` Dan Williams
0 siblings, 0 replies; 6+ messages in thread
From: Dan Williams @ 2022-01-05 3:22 UTC (permalink / raw)
To: Alison Schofield; +Cc: Ben Widawsky, Ira Weiny, Vishal Verma, linux-cxl
On Tue, Jan 4, 2022 at 4:54 PM Alison Schofield
<alison.schofield@intel.com> wrote:
>
> Thanks for the review Ben...
>
> On Tue, Jan 04, 2022 at 03:21:34PM -0800, Ben Widawsky wrote:
> > On 22-01-03 12:21:00, alison.schofield@intel.com wrote:
> > > From: Alison Schofield <alison.schofield@intel.com>
> > >
> > > User space may send the SET_PARTITION_INFO mailbox command using
> > > the IOCTL interface. Inspect the input payload and fail if the
> > > immediate flag is set.
> > >
> > > This is the first instance of the driver inspecting an input payload
> > > from user space. Assume there will be more such cases and implement
> > > with an extensible helper.
> >
> > Not sure if it's useful, but this was implemented at some point:
> > https://lore.kernel.org/linux-cxl/20210210000259.635748-8-ben.widawsky@intel.com/
>
> Thanks. Looking.
>
> >
> snip
>
> > > @@ -405,6 +440,14 @@ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds,
> > > }
> > > }
> > >
> > > + if (!cxl_payload_from_user_allowed(mbox_cmd.opcode,
> > > + mbox_cmd.payload_in)) {
> > > + dev_dbg(dev, "%s: input payload not allowed\n",
> > > + cxl_command_names[cmd->info.id].name);
> > > + rc = -EINVAL;
> > > + goto out;
> > > + }
> > > +
> >
> > Perhaps foolishly, the kdocs for handle_mailbox_cmd_from_user() documents the
> > error conditions. Would you mind adding EINVAL?
>
> I'm going to try to mv the allowable check, so this will go away.
Wouldn't the error code be EBUSY just like the other commands the
kernel blocks based on the state of the device?
It's moot if this is moving.
>
> >
> > Also, cxl_validate_cmd_from_user() was supposed to handle this kind of stuff.
> > All validation from user commands should spawn from that. Is there some reason
> > this one is different?
>
> The existing cxl_validate_cmd_from_user() happens before we copy the
> payload from user - so this payload check didn't fit in there.
> Let me look at reorganizing a bit so validating the cmd and it's payload
> can both spawn from cxl_validate_cmd_from_user().
Having cxl_validate_cmd_from_user() setup the 'struct cxl_mbox_cmd'
and then forward that along to handle_mailbox_cmd_from_user() doesn't
sound too bad. It does mean we need to remember that if the kernel
grows internal paths for calling commands that might be disallowed by
the state of the device we might need to move this validation out of
the "from user" context.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-05 3:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-01-03 20:21 [PATCH] cxl/mbox: Do not allow immediate mode in SET_PARTITION_INFO alison.schofield
2022-01-04 19:02 ` Dan Williams
2022-01-04 23:11 ` Alison Schofield
2022-01-04 23:21 ` Ben Widawsky
2022-01-05 0:59 ` Alison Schofield
2022-01-05 3:22 ` Dan Williams
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.