[PATCH] nvme: Fix NVME_IOCTL_ADMIN_CMD compat address handling.

[PATCH] nvme: Fix NVME_IOCTL_ADMIN_CMD compat address handling.

[Nick Bowler]

On a real 32-bit kernel, the upper bits of userspace addresses passed
to NVME_IOCTL_ADMIN_CMD via the nvme_passthru_cmd structure are silently
ignored by the nvme driver.

However on a 64-bit kernel running a compat task, these upper bits are
not ignored and are in fact required to be zero for the ioctl to work.

Unfortunately, this difference matters.  32-bit smartctl submits garbage
in these upper bits because it seems the pointer value it puts into the
nvme_passthru_cmd structure is sign extended.  This works fine on a real
32-bit kernel but fails on a 64-bit one because (at least on my setup)
the addresses smartctl uses are consistently above 2G.  For example:

  # smartctl -x /dev/nvme0n1p1
  smartctl 7.1 2019-12-30 r5022 [x86_64-linux-5.5.11] (local build)
  Copyright (C) 2002-19, Bruce Allen, Christian Franke, www.smartmontools.org

  Read NVMe Identify Controller failed: NVME_IOCTL_ADMIN_CMD: Bad address

Since changing 32-bit kernels to actually check all of the submitted
address bits now would break existing userspace, this patch fixes the
problem by explicitly zeroing the upper bits in the compat case.  This
enables 32-bit smartctl to work on a 64-bit kernel.

Signed-off-by: Nick Bowler <nbowler@draconx.ca>
---
 drivers/nvme/host/core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a4d8c90ee7cc..afb7b76d1d8a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -6,6 +6,7 @@
 
 #include <linux/blkdev.h>
 #include <linux/blk-mq.h>
+#include <linux/compat.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/hdreg.h>
@@ -1412,6 +1413,16 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	if (cmd.timeout_ms)
 		timeout = msecs_to_jiffies(cmd.timeout_ms);
 
+	if (in_compat_syscall()) {
+		/*
+		 * On real 32-bit kernels this implementation ignores the
+		 * upper bits of address fields so we must replicate that
+		 * behaviour in the compat case.
+		 */
+		cmd.addr = (compat_uptr_t)cmd.addr;
+		cmd.metadata = (compat_uptr_t)cmd.metadata;
+	}
+
 	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,
-- 
2.24.1

Re: [PATCH] nvme: Fix NVME_IOCTL_ADMIN_CMD compat address handling.

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH] nvme: Fix NVME_IOCTL_ADMIN_CMD compat address handling.

[Keith Busch]

Queued up for nvme-5.7, thanks.

Re: [PATCH] nvme: Fix NVME_IOCTL_ADMIN_CMD compat address handling.

[Christoph Hellwig]

A couple of comments:

No need for the "." the end of the subject line.

I also think you should just talk of the nvme_user_cmd function,
as that also is used for the NVME_IOCTL_IO_CMD ioctl.  Also there now
is a nvme_user_cmd64 variant that needs the same fix, can you also
include that?

> +	if (in_compat_syscall()) {
> +		/*
> +		 * On real 32-bit kernels this implementation ignores the
> +		 * upper bits of address fields so we must replicate that
> +		 * behaviour in the compat case.

s/real //g please, there are no fake 32-vit kernels :)

Re: [PATCH] nvme: Fix NVME_IOCTL_ADMIN_CMD compat address handling.

[Nick Bowler]

On 25/03/2020, Christoph Hellwig <hch@infradead.org> wrote:
> A couple of comments:
>
> No need for the "." the end of the subject line.
>
> I also think you should just talk of the nvme_user_cmd function,
> as that also is used for the NVME_IOCTL_IO_CMD ioctl.  Also there now
> is a nvme_user_cmd64 variant that needs the same fix, can you also
> include that?

Fair enough.  I can make the same change there... but I don't know how
to actually test the nvme_user_cmd64 function because I cannot find any
users of the NVME_IOCTL_ADMIN64_CMD or NVME_IOCTL_IO64_CMD ioctls.

>> +	if (in_compat_syscall()) {
>> +		/*
>> +		 * On real 32-bit kernels this implementation ignores the
>> +		 * upper bits of address fields so we must replicate that
>> +		 * behaviour in the compat case.
>
> s/real //g please, there are no fake 32-vit kernels :)

OK.

I shall prepare a v2 patch then with this feedback addressed.

Thanks,
  Nick

Re: [PATCH] nvme: Fix NVME_IOCTL_ADMIN_CMD compat address handling.

[Keith Busch]

On Wed, Mar 25, 2020 at 12:11:03PM -0700, Christoph Hellwig wrote:
> A couple of comments:
> 
> No need for the "." the end of the subject line.
> 
> I also think you should just talk of the nvme_user_cmd function,
> as that also is used for the NVME_IOCTL_IO_CMD ioctl.  Also there now
> is a nvme_user_cmd64 variant that needs the same fix, can you also
> include that?

Yes, this patch should get those cases too.

I unstaged this patch for now, could you send a v2 with the suggestions?

Re: [PATCH] nvme: Fix NVME_IOCTL_ADMIN_CMD compat address handling.

[Nick Bowler]

On 2020-03-25, Keith Busch <kbusch@kernel.org> wrote:
> On Wed, Mar 25, 2020 at 12:11:03PM -0700, Christoph Hellwig wrote:
>> A couple of comments:
>>
>> No need for the "." the end of the subject line.
>>
>> I also think you should just talk of the nvme_user_cmd function,
>> as that also is used for the NVME_IOCTL_IO_CMD ioctl.  Also there now
>> is a nvme_user_cmd64 variant that needs the same fix, can you also
>> include that?
>
> Yes, this patch should get those cases too.
>
> I unstaged this patch for now, could you send a v2 with the suggestions?

Will do.

Cheers,
  Nick