[PATCH 0/3] nvme-core: add user command filter

[PATCH 0/3] nvme-core: add user command filter

[Chaitanya Kulkarni]

This patch-series adds a filter to allow only certain user commands.                                   
Right now we use this infrastructure to prevent the user get log page
commands where RAE bit is cleared. We only allow get log pages to be
read with RAE == 0 where we issue the uevent to the userspace so that
user can clear the log pages.

Here we white list the log pages which are only allowed when RAE == 0.
We also allow Vendor Specific log pages irrespective of the RAE.

Chaitanya Kulkarni (3):
  nvme: introduce and use RAE bit macro
  nvme: add sanitize log identifier
  nvme-core: add filter for user commands

 drivers/nvme/host/core.c    | 64 ++++++++++++++++++++++++++++++++++---
 drivers/nvme/target/nvmet.h |  2 +-
 include/linux/nvme.h        |  3 ++
 3 files changed, 64 insertions(+), 5 deletions(-)

-- 
2.17.0

[PATCH 1/3] nvme: introduce and use RAE bit macro

[Chaitanya Kulkarni]

Introduce a macro instead of hardcoding RAE bit value in several places.
Also this uses RAE bit macro and removes the open coding in the NVMeOF
target. This is a preparation patch for filtering out the NVMe user
commands.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/target/nvmet.h | 2 +-
 include/linux/nvme.h        | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index c25d88fc9dec..47d9f5838828 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -343,7 +343,7 @@ struct nvmet_async_event {
 
 static inline void nvmet_clear_aen_bit(struct nvmet_req *req, u32 bn)
 {
-	int rae = le32_to_cpu(req->cmd->common.cdw10) & 1 << 15;
+	int rae = le32_to_cpu(req->cmd->common.cdw10) & NVME_LOG_PAGE_RAE;
 
 	if (!rae)
 		clear_bit(bn, &req->sq->ctrl->aen_masked);
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index c40720cb59ac..d439c129d8af 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -840,6 +840,8 @@ enum {
 	NVME_FWACT_ACTV		= (2 << 3),
 };
 
+#define NVME_LOG_PAGE_RAE	(1 << 15)
+
 /* NVMe Namespace Write Protect State */
 enum {
 	NVME_NS_NO_WRITE_PROTECT = 0,
-- 
2.17.0

[PATCH 2/3] nvme: add sanitize log identifier

[Chaitanya Kulkarni]

Introduce sanitize log page identifier which is used in next patch.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 include/linux/nvme.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index d439c129d8af..35381d689936 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -835,6 +835,7 @@ enum {
 	NVME_LOG_ANA		= 0x0c,
 	NVME_LOG_DISC		= 0x70,
 	NVME_LOG_RESERVATION	= 0x80,
+	NVME_LOG_SANITIZE       = 0x81,
 	NVME_FWACT_REPL		= (0 << 3),
 	NVME_FWACT_REPL_ACTV	= (1 << 3),
 	NVME_FWACT_ACTV		= (2 << 3),
-- 
2.17.0

[PATCH 3/3] nvme-core: add filter for user commands

[Chaitanya Kulkarni]

This patch adds a filter for the user commands. Right now we only check
for the NVMe Get Log Page commands. We only allow the user to issue get
log page command with RAE == 0 where we generate uevent for the async
event associated with this log page id.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/host/core.c | 64 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 60 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index eebaeadaa800..afd303489473 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1305,6 +1305,59 @@ static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects)
 		nvme_queue_scan(ctrl);
 }
 
+static inline bool nvme_get_log_page_allowed(struct nvme_command *c)
+{
+	bool ret;
+	u8 lid = c->get_log_page.lid;
+	int rae = le32_to_cpu(c->common.cdw10) & NVME_LOG_PAGE_RAE;
+
+	/* if rae == 1 or lid == Vendor specific allow get log page cmd */
+	if (rae || (lid >= 0xC && 0xFF <= lid))
+		return true;
+
+	switch (lid) {
+	/*
+	 * We send uevent for the following logs, See nvme_aen_uevent(), hence
+	 * allow only those log-pages to be cleared by the user for log page
+	 * rae == 0 command.
+	 */
+	case NVME_LOG_ERROR:
+	case NVME_LOG_SMART:
+	case NVME_LOG_RESERVATION:
+	case NVME_LOG_SANITIZE:
+	case NVME_LOG_DISC:
+		ret = true;
+		break;
+	default:
+		ret = false;
+	}
+	return ret;
+}
+
+static inline bool nvme_user_admin_cmd_allowed(struct nvme_command *c)
+{
+	bool ret;
+
+	switch (c->common.opcode) {
+	case nvme_admin_get_log_page:
+		ret = nvme_get_log_page_allowed(c);
+		break;
+	default:
+		ret = true;
+	}
+	return ret;
+}
+
+static inline bool nvme_user_cmd_allowed(struct nvme_ns *ns,
+		struct nvme_command *c)
+{
+	/*
+	 * Right now only check for admin commands, in future for I/O commands
+	 * please introduce nvme_user_io_cmd_allowed().
+	 */
+	return ns == NULL ? nvme_user_admin_cmd_allowed(c) : true;
+}
+
 static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 			struct nvme_passthru_cmd __user *ucmd)
 {
@@ -1337,6 +1390,10 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	if (cmd.timeout_ms)
 		timeout = msecs_to_jiffies(cmd.timeout_ms);
 
+	/* use nvme_command to make code more readable */
+	if (!nvme_user_cmd_allowed(ns, &c))
+		return -EINVAL;
+
 	effects = nvme_passthru_start(ctrl, ns, cmd.opcode);
 	status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
 			(void __user *)(uintptr_t)cmd.addr, cmd.data_len,
@@ -3434,10 +3491,9 @@ static void nvme_clear_changed_ns_log(struct nvme_ctrl *ctrl)
 		return;
 
 	/*
-	 * We need to read the log to clear the AEN, but we don't want to rely
-	 * on it for the changed namespace information as userspace could have
-	 * raced with us in reading the log page, which could cause us to miss
-	 * updates.
+	 * We don't allow userspace to clear this log page as we filter the user
+	 * log page command with RAE == 0, see nvme_user_admin_cmd_allowed().
+	 * Now we can rely on this updated log page information.
 	 */
 	error = nvme_get_log(ctrl, NVME_NSID_ALL, NVME_LOG_CHANGED_NS, 0, log,
 			log_size, 0);
-- 
2.17.0

[PATCH 0/3] nvme-core: add user command filter

[Chaitanya Kulkarni]

Gentle ping.

On 05/08/2019 11:36 AM, Chaitanya Kulkarni wrote:
> This patch-series adds a filter to allow only certain user commands.
> Right now we use this infrastructure to prevent the user get log page
> commands where RAE bit is cleared. We only allow get log pages to be
> read with RAE == 0 where we issue the uevent to the userspace so that
> user can clear the log pages.
>
> Here we white list the log pages which are only allowed when RAE == 0.
> We also allow Vendor Specific log pages irrespective of the RAE.
>
> Chaitanya Kulkarni (3):
>    nvme: introduce and use RAE bit macro
>    nvme: add sanitize log identifier
>    nvme-core: add filter for user commands
>
>   drivers/nvme/host/core.c    | 64 ++++++++++++++++++++++++++++++++++---
>   drivers/nvme/target/nvmet.h |  2 +-
>   include/linux/nvme.h        |  3 ++
>   3 files changed, 64 insertions(+), 5 deletions(-)
>

[PATCH 0/3] nvme-core: add user command filter

[Keith Busch]

On Wed, May 08, 2019@11:36:31AM -0700, Chaitanya Kulkarni wrote:
> This patch-series adds a filter to allow only certain user commands.                                   
> Right now we use this infrastructure to prevent the user get log page
> commands where RAE bit is cleared. We only allow get log pages to be
> read with RAE == 0 where we issue the uevent to the userspace so that
> user can clear the log pages.
> 
> Here we white list the log pages which are only allowed when RAE == 0.
> We also allow Vendor Specific log pages irrespective of the RAE.

I'm generally against the passthrough interface examining commands. It
is not for the driver to decide what an admin can't send to their devices.

[PATCH 0/3] nvme-core: add user command filter

[Christoph Hellwig]

On Fri, May 10, 2019@03:42:56PM -0600, Keith Busch wrote:
> On Wed, May 08, 2019@11:36:31AM -0700, Chaitanya Kulkarni wrote:
> > This patch-series adds a filter to allow only certain user commands.                                   
> > Right now we use this infrastructure to prevent the user get log page
> > commands where RAE bit is cleared. We only allow get log pages to be
> > read with RAE == 0 where we issue the uevent to the userspace so that
> > user can clear the log pages.
> > 
> > Here we white list the log pages which are only allowed when RAE == 0.
> > We also allow Vendor Specific log pages irrespective of the RAE.
> 
> I'm generally against the passthrough interface examining commands. It
> is not for the driver to decide what an admin can't send to their devices.

Well, the whole AER model is based around log pages clearing the
event, so if userspace clears these events we are in pretty deep
trouble.  Would you prefer just setting the RAE bit unconditionally
for these log pages?

[PATCH 0/3] nvme-core: add user command filter

[Keith Busch]

On Mon, May 13, 2019@06:25:17AM -0700, Christoph Hellwig wrote:
> On Fri, May 10, 2019@03:42:56PM -0600, Keith Busch wrote:
> > On Wed, May 08, 2019@11:36:31AM -0700, Chaitanya Kulkarni wrote:
> > > This patch-series adds a filter to allow only certain user commands.                                   
> > > Right now we use this infrastructure to prevent the user get log page
> > > commands where RAE bit is cleared. We only allow get log pages to be
> > > read with RAE == 0 where we issue the uevent to the userspace so that
> > > user can clear the log pages.
> > > 
> > > Here we white list the log pages which are only allowed when RAE == 0.
> > > We also allow Vendor Specific log pages irrespective of the RAE.
> > 
> > I'm generally against the passthrough interface examining commands. It
> > is not for the driver to decide what an admin can't send to their devices.
> 
> Well, the whole AER model is based around log pages clearing the
> event, so if userspace clears these events we are in pretty deep
> trouble.  Would you prefer just setting the RAE bit unconditionally
> for these log pages?

What if user space really wants to clear it? We shouldn't just make that
capability unreachable to admins.

[PATCH 0/3] nvme-core: add user command filter

[Chaitanya Kulkarni]

Hi Keith,
On 5/13/19 9:54 AM, Keith Busch wrote:
> On Mon, May 13, 2019@06:25:17AM -0700, Christoph Hellwig wrote:
>> On Fri, May 10, 2019@03:42:56PM -0600, Keith Busch wrote:
>>> On Wed, May 08, 2019@11:36:31AM -0700, Chaitanya Kulkarni wrote:
>>>> This patch-series adds a filter to allow only certain user commands.                                   
>>>> Right now we use this infrastructure to prevent the user get log page
>>>> commands where RAE bit is cleared. We only allow get log pages to be
>>>> read with RAE == 0 where we issue the uevent to the userspace so that
>>>> user can clear the log pages.
>>>>
>>>> Here we white list the log pages which are only allowed when RAE == 0.
>>>> We also allow Vendor Specific log pages irrespective of the RAE.
>>> I'm generally against the passthrough interface examining commands. It
>>> is not for the driver to decide what an admin can't send to their devices.
>> Well, the whole AER model is based around log pages clearing the
>> event, so if userspace clears these events we are in pretty deep
>> trouble.  Would you prefer just setting the RAE bit unconditionally
>> for these log pages?
> What if user space really wants to clear it? We shouldn't just make that
> capability unreachable to admins.
>

I agree that passthru means we should just passthru. But user-pace
doesn't have the access to take

corrective action on clearing the AEN.

The cases where Kernel is responsible for reading and clearing out the
log pages/AENs

if we allow userspace to clear those it will create a problem in the
kernel code if kernel code

expects the aen bit to be set. This can lead to complex bugs where,

1. Kernel expects the log page and aen to be present.

2. But it is been cleared since userspace read it but did not take the
corrective action or doesn't have the

    right interface to take corrective action so it depends on a kernel
(driver) to take the corrective action.

[PATCH 0/3] nvme-core: add user command filter

[Keith Busch]

On Mon, May 13, 2019@08:36:32PM +0000, Chaitanya Kulkarni wrote:
> I agree that passthru means we should just passthru. But user-pace
> doesn't have the access to take
> 
> corrective action on clearing the AEN.
> 
> The cases where Kernel is responsible for reading and clearing out the
> log pages/AENs
> 
> if we allow userspace to clear those it will create a problem in the
> kernel code if kernel code
> 
> expects the aen bit to be set. This can lead to complex bugs where,
> 
> 1. Kernel expects the log page and aen to be present.
> 
> 2. But it is been cleared since userspace read it but did not take the
> corrective action or doesn't have the
> 
>     right interface to take corrective action so it depends on a kernel
> (driver) to take the corrective action.

But the kernel still sees the AEN, and the log the kernel wants to read
will still exist. None of the logs contents are latched to RAE, so I'm
afraid I'm not seeing what problem letting user space re-arm the AEN
creates. Could you walk me through a scenario that triggers such a bug?