[PATCH v5] nvme: reject passthrough of driver-managed Set Features

[PATCH v5] nvme: reject passthrough of driver-managed Set Features

[Chao Shi]

Since commit b58da2d270db ("nvme: update keep alive interval when kato
is modified"), userspace can start keep-alive on any transport via a
Set Features (KATO) passthrough command. nvme_keep_alive_work() then
allocates with BLK_MQ_REQ_RESERVED, but nvme_alloc_admin_tag_set()
only reserves admin tags for fabrics, so the allocation trips
WARN_ON_ONCE() in blk_mq_get_tag() and fails:

  nvme nvme0: keep-alive failed: -11

More generally, several Set Features change controller state that the
driver manages itself and cannot react to correctly when set behind
its back from userspace. Reject these in nvme_cmd_allowed():

  - KATO on non-fabrics (keep-alive is only armed for fabrics; on PCIe
    it has no reserved tag and an active keep-alive harms idle power
    states)
  - Host Behavior Support, Host Memory Buffer, Number of Queues, and
    Autonomous Power State Transition (all driver-managed)

Keep Alive on fabrics is unchanged. I/O commands are unaffected as the
check is confined to the admin path (ns == NULL).

Link: https://lore.kernel.org/linux-nvme/20260522162639.395802-1-coshi036@gmail.com/

Fixes: b58da2d270db ("nvme: update keep alive interval when kato is modified")

Found by FuzzNvme(Syzkaller with FEMU fuzzing framework).

Acked-by: Sungwoo Kim <iam@sung-woo.kim>
Acked-by: Dave Tian <daveti@purdue.edu>
Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
---

Reproducer for the keep-alive case (run as root on a PCIe NVMe device):

    #include <fcntl.h>
    #include <stdio.h>
    #include <string.h>
    #include <sys/ioctl.h>
    #include <linux/nvme_ioctl.h>

    int main(void)
    {
            struct nvme_admin_cmd cmd = {0};
            int fd = open("/dev/nvme0", O_RDWR);
            if (fd < 0) { perror("open"); return 1; }
            cmd.opcode = 0x09;       /* SET_FEATURES */
            cmd.cdw10  = 0x0f;       /* Feature ID: KATO */
            cmd.cdw11  = 5;          /* KATO = 5 seconds */
            if (ioctl(fd, NVME_IOCTL_ADMIN_CMD, &cmd) < 0) {
                    perror("ioctl");
                    return 1;
            }
            return 0;
    }

On an unpatched kernel, within ~kato/2 seconds after the program exits,
dmesg shows:

    nvme nvme0: keep alive interval updated from 0 ms to 5000 ms
    WARNING: CPU: 0 PID: ... at block/blk-mq-tag.c:148 blk_mq_get_tag+...
    nvme nvme0: keep-alive failed: -11

With this patch the ioctl fails with EACCES on non-fabrics.

Changes since v4:
- Fold the check into the existing nvme_cmd_allowed() instead of a
  separate helper, and reject additional driver-managed Set Features
  (Host Behavior, Host Memory Buffer, Number of Queues, Autonomous
  Power State Transition) in the same switch (Keith Busch). The admin
  vs I/O distinction is now structural: the switch lives in the
  ns == NULL branch, so I/O commands (e.g. Dataset Management, which
  shares opcode 0x09 with Set Features) are never inspected.

Changes since v3:
- Only inspect admin commands so a DSM I/O command is not wrongly
  rejected (Keith Busch).

Changes since v2:
- Reject the KATO passthrough on non-fabrics instead of reserving an
  admin tag for all transports (Keith Busch, Christoph Hellwig).

Changes since v1:
- v2 added a spec citation and quirk discussion, superseded by the
  reject approach.

 drivers/nvme/host/ioctl.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index a9c097dacad6..31784506e845 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -14,8 +14,9 @@ enum {
 	NVME_IOCTL_PARTITION	= (1 << 1),
 };
 
-static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
-		unsigned int flags, bool open_for_write)
+static bool nvme_cmd_allowed(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
+			     struct nvme_command *c, unsigned int flags,
+			     bool open_for_write)
 {
 	u32 effects;
 
@@ -50,6 +51,26 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 			case NVME_ID_CNS_CTRL:
 				return true;
 			}
+		} else if (c->common.opcode == nvme_admin_set_features) {
+			/*
+			 * Reject Set Features that change controller state the
+			 * driver manages itself; setting them behind the driver's
+			 * back from userspace leaves it unable to react correctly.
+			 * Keep Alive is only armed for fabrics - on other
+			 * transports it has no reserved tag and harms idle power
+			 * states.
+			 */
+			switch (le32_to_cpu(c->features.fid) & 0xff) {
+			case NVME_FEAT_KATO:
+				if (ctrl->ops->flags & NVME_F_FABRICS)
+					break;
+				fallthrough;
+			case NVME_FEAT_HOST_BEHAVIOR:
+			case NVME_FEAT_HOST_MEM_BUF:
+			case NVME_FEAT_NUM_QUEUES:
+			case NVME_FEAT_AUTO_PST:
+				return false;
+			}
 		}
 		goto admin;
 	}
@@ -59,7 +80,7 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 	 * and marks this command as supported.  If not reject unprivileged
 	 * passthrough.
 	 */
-	effects = nvme_command_effects(ns->ctrl, ns, c->common.opcode);
+	effects = nvme_command_effects(ctrl, ns, c->common.opcode);
 	if (!(effects & NVME_CMD_EFFECTS_CSUPP))
 		goto admin;
 
@@ -308,7 +329,7 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	c.common.cdw14 = cpu_to_le32(cmd.cdw14);
 	c.common.cdw15 = cpu_to_le32(cmd.cdw15);
 
-	if (!nvme_cmd_allowed(ns, &c, 0, open_for_write))
+	if (!nvme_cmd_allowed(ctrl, ns, &c, 0, open_for_write))
 		return -EACCES;
 
 	if (cmd.timeout_ms)
@@ -355,7 +376,7 @@ static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	c.common.cdw14 = cpu_to_le32(cmd.cdw14);
 	c.common.cdw15 = cpu_to_le32(cmd.cdw15);
 
-	if (!nvme_cmd_allowed(ns, &c, flags, open_for_write))
+	if (!nvme_cmd_allowed(ctrl, ns, &c, flags, open_for_write))
 		return -EACCES;
 
 	if (cmd.timeout_ms)
@@ -472,7 +493,7 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	c.common.cdw14 = cpu_to_le32(READ_ONCE(cmd->cdw14));
 	c.common.cdw15 = cpu_to_le32(READ_ONCE(cmd->cdw15));
 
-	if (!nvme_cmd_allowed(ns, &c, 0, ioucmd->file->f_mode & FMODE_WRITE))
+	if (!nvme_cmd_allowed(ctrl, ns, &c, 0, ioucmd->file->f_mode & FMODE_WRITE))
 		return -EACCES;
 
 	d.metadata = READ_ONCE(cmd->metadata);
-- 
2.43.0

Re: [PATCH v5] nvme: reject passthrough of driver-managed Set Features

[Tokunori Ikegami]

On 2026/05/24 7:56, Chao Shi wrote:
> Since commit b58da2d270db ("nvme: update keep alive interval when kato
> is modified"), userspace can start keep-alive on any transport via a
> Set Features (KATO) passthrough command. nvme_keep_alive_work() then
> allocates with BLK_MQ_REQ_RESERVED, but nvme_alloc_admin_tag_set()
> only reserves admin tags for fabrics, so the allocation trips
> WARN_ON_ONCE() in blk_mq_get_tag() and fails:
>
>    nvme nvme0: keep-alive failed: -11
>
> More generally, several Set Features change controller state that the
> driver manages itself and cannot react to correctly when set behind
> its back from userspace. Reject these in nvme_cmd_allowed():
>
>    - KATO on non-fabrics (keep-alive is only armed for fabrics; on PCIe
>      it has no reserved tag and an active keep-alive harms idle power
>      states)
>    - Host Behavior Support, Host Memory Buffer, Number of Queues, and
>      Autonomous Power State Transition (all driver-managed)
>
> Keep Alive on fabrics is unchanged. I/O commands are unaffected as the
> check is confined to the admin path (ns == NULL).
>
> Link: https://lore.kernel.org/linux-nvme/20260522162639.395802-1-coshi036@gmail.com/
>
> Fixes: b58da2d270db ("nvme: update keep alive interval when kato is modified")
>
> Found by FuzzNvme(Syzkaller with FEMU fuzzing framework).
>
> Acked-by: Sungwoo Kim <iam@sung-woo.kim>
> Acked-by: Dave Tian <daveti@purdue.edu>
> Acked-by: Weidong Zhu <weizhu@fiu.edu>
> Signed-off-by: Chao Shi <coshi036@gmail.com>
> ---
>
> Reproducer for the keep-alive case (run as root on a PCIe NVMe device):
>
>      #include <fcntl.h>
>      #include <stdio.h>
>      #include <string.h>
>      #include <sys/ioctl.h>
>      #include <linux/nvme_ioctl.h>
>
>      int main(void)
>      {
>              struct nvme_admin_cmd cmd = {0};
>              int fd = open("/dev/nvme0", O_RDWR);
>              if (fd < 0) { perror("open"); return 1; }
>              cmd.opcode = 0x09;       /* SET_FEATURES */
>              cmd.cdw10  = 0x0f;       /* Feature ID: KATO */
>              cmd.cdw11  = 5;          /* KATO = 5 seconds */
>              if (ioctl(fd, NVME_IOCTL_ADMIN_CMD, &cmd) < 0) {
>                      perror("ioctl");
>                      return 1;
>              }
>              return 0;
>      }
>
> On an unpatched kernel, within ~kato/2 seconds after the program exits,
> dmesg shows:
>
>      nvme nvme0: keep alive interval updated from 0 ms to 5000 ms
>      WARNING: CPU: 0 PID: ... at block/blk-mq-tag.c:148 blk_mq_get_tag+...
>      nvme nvme0: keep-alive failed: -11
>
> With this patch the ioctl fails with EACCES on non-fabrics.
>
> Changes since v4:
> - Fold the check into the existing nvme_cmd_allowed() instead of a
>    separate helper, and reject additional driver-managed Set Features
>    (Host Behavior, Host Memory Buffer, Number of Queues, Autonomous
>    Power State Transition) in the same switch (Keith Busch). The admin
>    vs I/O distinction is now structural: the switch lives in the
>    ns == NULL branch, so I/O commands (e.g. Dataset Management, which
>    shares opcode 0x09 with Set Features) are never inspected.
>
> Changes since v3:
> - Only inspect admin commands so a DSM I/O command is not wrongly
>    rejected (Keith Busch).
>
> Changes since v2:
> - Reject the KATO passthrough on non-fabrics instead of reserving an
>    admin tag for all transports (Keith Busch, Christoph Hellwig).
>
> Changes since v1:
> - v2 added a spec citation and quirk discussion, superseded by the
>    reject approach.
>
>   drivers/nvme/host/ioctl.c | 33 +++++++++++++++++++++++++++------
>   1 file changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index a9c097dacad6..31784506e845 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -14,8 +14,9 @@ enum {
>   	NVME_IOCTL_PARTITION	= (1 << 1),
>   };
>   
> -static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
> -		unsigned int flags, bool open_for_write)
> +static bool nvme_cmd_allowed(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
> +			     struct nvme_command *c, unsigned int flags,
> +			     bool open_for_write)

The struct nvme_ns does already have the struct nvme_ctrl *ctrl as a 
member variable as below so seems not necessary to add the function 
argument struct nvme_ctrl *ctrl.
struct nvme_ns {

     struct list_head list;

     struct nvme_ctrl *ctrl;

Re: [PATCH v5] nvme: reject passthrough of driver-managed Set Features

[Keith Busch]

On Tue, May 26, 2026 at 12:34:03AM +0900, Tokunori Ikegami wrote:
> > +static bool nvme_cmd_allowed(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
> > +			     struct nvme_command *c, unsigned int flags,
> > +			     bool open_for_write)
> 
> The struct nvme_ns does already have the struct nvme_ctrl *ctrl as a member
> variable as below so seems not necessary to add the function argument struct
> nvme_ctrl *ctrl.

This filter is for admin commands, so the 'ns' parameter is NULL for
this case.

Re: [PATCH v5] nvme: reject passthrough of driver-managed Set Features

[Tokunori Ikegami]

On 2026/05/26 0:49, Keith Busch wrote:
> On Tue, May 26, 2026 at 12:34:03AM +0900, Tokunori Ikegami wrote:
>>> +static bool nvme_cmd_allowed(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
>>> +			     struct nvme_command *c, unsigned int flags,
>>> +			     bool open_for_write)
>> The struct nvme_ns does already have the struct nvme_ctrl *ctrl as a member
>> variable as below so seems not necessary to add the function argument struct
>> nvme_ctrl *ctrl.
> This filter is for admin commands, so the 'ns' parameter is NULL for
> this case.
Okay so how about that to use the function argument unsigned int flags 
instead with adding the NVME_IOCTL_FABRICS enum definition?
enum {
     NVME_IOCTL_VEC        = (1 << 0),
     NVME_IOCTL_PARTITION    = (1 << 1),
};
Note: I mean as possible as we can it seems better to avoid the many 
function arguments for the maintainability.
Thank you.

Re: [PATCH v5] nvme: reject passthrough of driver-managed Set Features

[Christoph Hellwig]

On Sat, May 23, 2026 at 06:56:29PM -0400, Chao Shi wrote:
> Since commit b58da2d270db ("nvme: update keep alive interval when kato
> is modified"), userspace can start keep-alive on any transport via a
> Set Features (KATO) passthrough command. nvme_keep_alive_work() then
> allocates with BLK_MQ_REQ_RESERVED, but nvme_alloc_admin_tag_set()
> only reserves admin tags for fabrics, so the allocation trips
> WARN_ON_ONCE() in blk_mq_get_tag() and fails:
> 
>   nvme nvme0: keep-alive failed: -11
> 
> More generally, several Set Features change controller state that the
> driver manages itself and cannot react to correctly when set behind
> its back from userspace. Reject these in nvme_cmd_allowed():
> 
>   - KATO on non-fabrics (keep-alive is only armed for fabrics; on PCIe
>     it has no reserved tag and an active keep-alive harms idle power
>     states)
>   - Host Behavior Support, Host Memory Buffer, Number of Queues, and
>     Autonomous Power State Transition (all driver-managed)
> 
> Keep Alive on fabrics is unchanged. I/O commands are unaffected as the
> check is confined to the admin path (ns == NULL).
> 
> Link: https://lore.kernel.org/linux-nvme/20260522162639.395802-1-coshi036@gmail.com/
> 
> Fixes: b58da2d270db ("nvme: update keep alive interval when kato is modified")
> 
> Found by FuzzNvme(Syzkaller with FEMU fuzzing framework).
> 
> Acked-by: Sungwoo Kim <iam@sung-woo.kim>
> Acked-by: Dave Tian <daveti@purdue.edu>
> Acked-by: Weidong Zhu <weizhu@fiu.edu>
> Signed-off-by: Chao Shi <coshi036@gmail.com>
> ---
> 
> Reproducer for the keep-alive case (run as root on a PCIe NVMe device):
> 
>     #include <fcntl.h>
>     #include <stdio.h>
>     #include <string.h>
>     #include <sys/ioctl.h>
>     #include <linux/nvme_ioctl.h>
> 
>     int main(void)
>     {
>             struct nvme_admin_cmd cmd = {0};
>             int fd = open("/dev/nvme0", O_RDWR);
>             if (fd < 0) { perror("open"); return 1; }
>             cmd.opcode = 0x09;       /* SET_FEATURES */
>             cmd.cdw10  = 0x0f;       /* Feature ID: KATO */
>             cmd.cdw11  = 5;          /* KATO = 5 seconds */
>             if (ioctl(fd, NVME_IOCTL_ADMIN_CMD, &cmd) < 0) {
>                     perror("ioctl");
>                     return 1;
>             }
>             return 0;
>     }
> 
> On an unpatched kernel, within ~kato/2 seconds after the program exits,
> dmesg shows:
> 
>     nvme nvme0: keep alive interval updated from 0 ms to 5000 ms
>     WARNING: CPU: 0 PID: ... at block/blk-mq-tag.c:148 blk_mq_get_tag+...
>     nvme nvme0: keep-alive failed: -11
> 
> With this patch the ioctl fails with EACCES on non-fabrics.
> 
> Changes since v4:
> - Fold the check into the existing nvme_cmd_allowed() instead of a
>   separate helper, and reject additional driver-managed Set Features
>   (Host Behavior, Host Memory Buffer, Number of Queues, Autonomous
>   Power State Transition) in the same switch (Keith Busch). The admin
>   vs I/O distinction is now structural: the switch lives in the
>   ns == NULL branch, so I/O commands (e.g. Dataset Management, which
>   shares opcode 0x09 with Set Features) are never inspected.
> 
> Changes since v3:
> - Only inspect admin commands so a DSM I/O command is not wrongly
>   rejected (Keith Busch).
> 
> Changes since v2:
> - Reject the KATO passthrough on non-fabrics instead of reserving an
>   admin tag for all transports (Keith Busch, Christoph Hellwig).
> 
> Changes since v1:
> - v2 added a spec citation and quirk discussion, superseded by the
>   reject approach.
> 
>  drivers/nvme/host/ioctl.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index a9c097dacad6..31784506e845 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -14,8 +14,9 @@ enum {
>  	NVME_IOCTL_PARTITION	= (1 << 1),
>  };
>  
> -static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
> -		unsigned int flags, bool open_for_write)
> +static bool nvme_cmd_allowed(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
> +			     struct nvme_command *c, unsigned int flags,
> +			     bool open_for_write)
>  {
>  	u32 effects;
>  
> @@ -50,6 +51,26 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
>  			case NVME_ID_CNS_CTRL:
>  				return true;
>  			}
> +		} else if (c->common.opcode == nvme_admin_set_features) {
> +			/*
> +			 * Reject Set Features that change controller state the
> +			 * driver manages itself; setting them behind the driver's
> +			 * back from userspace leaves it unable to react correctly.

Overly long lines.  I suspect we're best off splitting out the
admin and ns-command set specific parts of nvme_cmd_allowed into
separate helpers.  And maybe use a switch statement on the command
as nested ifs become cumersome in the long run.

> -	if (!nvme_cmd_allowed(ns, &c, 0, ioucmd->file->f_mode & FMODE_WRITE))
> +	if (!nvme_cmd_allowed(ctrl, ns, &c, 0, ioucmd->file->f_mode & FMODE_WRITE))

Another overly long line here.

Otherwise this looks good.

Re: [PATCH v5] nvme: reject passthrough of driver-managed Set Features

[Keith Busch]

On Sat, May 23, 2026 at 06:56:29PM -0400, Chao Shi wrote:
> +			switch (le32_to_cpu(c->features.fid) & 0xff) {
> +			case NVME_FEAT_KATO:
> +				if (ctrl->ops->flags & NVME_F_FABRICS)
> +					break;
> +				fallthrough;
> +			case NVME_FEAT_HOST_BEHAVIOR:
> +			case NVME_FEAT_HOST_MEM_BUF:
> +			case NVME_FEAT_NUM_QUEUES:
> +			case NVME_FEAT_AUTO_PST:

I may have been overly restrictive with suggesting AUTO_PST for this
filter. Messing with the other features will break something, but power
state is just user policy. The driver may undo the user setting on a
controller reset, but so what?

Re: [PATCH v5] nvme: reject passthrough of driver-managed Set Features

[Christoph Hellwig]

On Wed, May 27, 2026 at 08:32:08AM -0600, Keith Busch wrote:
> On Sat, May 23, 2026 at 06:56:29PM -0400, Chao Shi wrote:
> > +			switch (le32_to_cpu(c->features.fid) & 0xff) {
> > +			case NVME_FEAT_KATO:
> > +				if (ctrl->ops->flags & NVME_F_FABRICS)
> > +					break;
> > +				fallthrough;
> > +			case NVME_FEAT_HOST_BEHAVIOR:
> > +			case NVME_FEAT_HOST_MEM_BUF:
> > +			case NVME_FEAT_NUM_QUEUES:
> > +			case NVME_FEAT_AUTO_PST:
> 
> I may have been overly restrictive with suggesting AUTO_PST for this
> filter. Messing with the other features will break something, but power
> state is just user policy. The driver may undo the user setting on a
> controller reset, but so what?

There's really no point in allowing it.  Just as we should have never
allowed low-level config of any kind, but unfortunately nvme admin
commands are a horrible grab all.