[PATCH v2 1/2] nvme: fix memory corruption for passthrough metadata

[PATCH v2 1/2] nvme: fix memory corruption for passthrough metadata

[Kanchan Joshi]

User can specify a smaller meta buffer than what the device is
wired to update/access.
This may lead to Device doing a larger DMA operation, overwriting
unrelated kernel memory.

Detect this situation for uring passthrough, and bail out.

Fixes: 456cba386e94 ("nvme: wire-up uring-cmd support for io-passthru on char-device")
Cc: stable@vger.kernel.org

Reported-by: Vincent Fu <vincent.fu@samsung.com>
Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---
 drivers/nvme/host/ioctl.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 19a5177bc360..717c7effaf8a 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -320,6 +320,35 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 			meta_len, lower_32_bits(io.slba), NULL, 0, 0);
 }
 
+static bool nvme_validate_passthru_meta(struct nvme_ctrl *ctrl,
+					struct nvme_ns *ns,
+					struct nvme_command *c,
+					__u64 meta, __u32 meta_len)
+{
+	/*
+	 * User may specify smaller meta-buffer with a larger data-buffer.
+	 * Driver allocated meta buffer will also be small.
+	 * Device can do larger dma into that, overwriting unrelated kernel
+	 * memory.
+	 */
+	if (ns && (meta_len || meta)) {
+		u16 nlb = lower_16_bits(le32_to_cpu(c->common.cdw12));
+		u16 control = upper_16_bits(le32_to_cpu(c->common.cdw12));
+
+		/* meta transfer from/to host is not done */
+		if (control & NVME_RW_PRINFO_PRACT && ns->ms == ns->pi_size)
+			return true;
+
+		if (meta_len != (nlb + 1) * ns->ms) {
+			dev_err(ctrl->device,
+			"%s: metadata length does not match!\n", current->comm);
+			return false;
+		}
+	}
+
+	return true;
+}
+
 static bool nvme_validate_passthru_nsid(struct nvme_ctrl *ctrl,
 					struct nvme_ns *ns, __u32 nsid)
 {
@@ -593,6 +622,10 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	d.metadata_len = READ_ONCE(cmd->metadata_len);
 	d.timeout_ms = READ_ONCE(cmd->timeout_ms);
 
+	if (!nvme_validate_passthru_meta(ctrl, ns, &c, d.metadata,
+					 d.metadata_len))
+		return -EINVAL;
+
 	if (issue_flags & IO_URING_F_NONBLOCK) {
 		rq_flags |= REQ_NOWAIT;
 		blk_flags = BLK_MQ_REQ_NOWAIT;
-- 
2.25.1

Re: [PATCH v2 1/2] nvme: fix memory corruption for passthrough metadata

[Vincent Fu]

I think the metadata size check is too strict. Commands where the metadata size
is too small should result in errors but when the metadata size is larger than
needed they should still go through.

In any case, I tested this patch on a QEMU NVMe device (which supports PI by
default).

I formatted the device with a 512+16 lbaf with a separate buffer for metadata:

nvme format /dev/ng0n1 -m 0 -i 1 -p 0 --lbaf 2 --force

Using the latest fio I wrote some data to it:

./fio --name=difdix --ioengine=io_uring_cmd --cmd_type=nvme \
  --filename=/dev/ng0n1 --rw=write --bs=512 --md_per_io_size=16 --pi_act=1 \
  --pi_chk=APPTAG --apptag=0x8888 --apptag_mask=0xFFFF --number_ios=128

Then I wrote a small program to read 4096 bytes from the device with only a
16-byte (instead of 64-byte) metadata buffer. Without this patch the kernel
crashes. With the patch the read fails with an error message in the kernel log.

Tested-by: Vincent Fu <vincent.fu@samsung.com>

Re: [PATCH v2 1/2] nvme: fix memory corruption for passthrough metadata

[KANCHAN JOSHI/Host Software /SSIR/Staff Engineer/Samsung Electronics]

On 8/31/2023 7:39 PM, Vincent Fu wrote:
> I think the metadata size check is too strict. Commands where the metadata size
> is too small should result in errors but when the metadata size is larger than
> needed they should still go through.

Indeed.
I will fold that change in the next version.

> In any case, I tested this patch on a QEMU NVMe device (which supports PI by
> default).
> 
> I formatted the device with a 512+16 lbaf with a separate buffer for metadata:
> 
> nvme format /dev/ng0n1 -m 0 -i 1 -p 0 --lbaf 2 --force
> 
> Using the latest fio I wrote some data to it:
> 
> ./fio --name=difdix --ioengine=io_uring_cmd --cmd_type=nvme \
>    --filename=/dev/ng0n1 --rw=write --bs=512 --md_per_io_size=16 --pi_act=1 \
>    --pi_chk=APPTAG --apptag=0x8888 --apptag_mask=0xFFFF --number_ios=128
> 
> Then I wrote a small program to read 4096 bytes from the device with only a
> 16-byte (instead of 64-byte) metadata buffer. Without this patch the kernel
> crashes. With the patch the read fails with an error message in the kernel log.
> 
> Tested-by: Vincent Fu <vincent.fu@samsung.com>

Thanks.

Re: [PATCH v2 1/2] nvme: fix memory corruption for passthrough metadata

[Keith Busch]

On Mon, Aug 14, 2023 at 12:32:12PM +0530, Kanchan Joshi wrote:
> +static bool nvme_validate_passthru_meta(struct nvme_ctrl *ctrl,
> +					struct nvme_ns *ns,
> +					struct nvme_command *c,
> +					__u64 meta, __u32 meta_len)
> +{
> +	/*
> +	 * User may specify smaller meta-buffer with a larger data-buffer.
> +	 * Driver allocated meta buffer will also be small.
> +	 * Device can do larger dma into that, overwriting unrelated kernel
> +	 * memory.
> +	 */

What if the user doesn't specify metadata or length for a command that
uses it? The driver won't set MPTR in that case, causing the device to
access NULL.

And similiar to this problem, what if the metadata is extended rather
than separate, and the user's buffer is too short? That will lead to the
same type of problem you're trying to fix here?

My main concern, though, is forward and backward compatibility. Even
when metadata is enabled, there are IO commands that don't touch it, so
some tool that erroneously requested it will stop working. Or perhaps
some other future opcode will have some other metadata use that doesn't
match up exactly with how read/write/compare/append use it. As much as
I'd like to avoid bad user commands from crashing, these kinds of checks
can become problematic for maintenance. I realize we already do similiar
sanity checks in nvme_submit_io(), but that one is confined to only 3
opcodes where this interface you're changing is much more flexible.

Re: [PATCH v2 1/2] nvme: fix memory corruption for passthrough metadata

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 2355 bytes --]

On Fri, Sep 01, 2023 at 10:45:50AM -0400, Keith Busch wrote:
>On Mon, Aug 14, 2023 at 12:32:12PM +0530, Kanchan Joshi wrote:
>> +static bool nvme_validate_passthru_meta(struct nvme_ctrl *ctrl,
>> +					struct nvme_ns *ns,
>> +					struct nvme_command *c,
>> +					__u64 meta, __u32 meta_len)
>> +{
>> +	/*
>> +	 * User may specify smaller meta-buffer with a larger data-buffer.
>> +	 * Driver allocated meta buffer will also be small.
>> +	 * Device can do larger dma into that, overwriting unrelated kernel
>> +	 * memory.
>> +	 */
>
>What if the user doesn't specify metadata or length for a command that
>uses it? The driver won't set MPTR in that case, causing the device to
>access NULL.

That is fine, because kernel is not going to allocate memory for that
case. I am not trying to provide any saftey net to user-space in this
patch. Rather, the objective is to prevent kernel-memory corruption.

>And similiar to this problem, what if the metadata is extended rather
>than separate, and the user's buffer is too short? That will lead to the
>same type of problem you're trying to fix here?

No.
For extended metadata, userspace is using its own buffer. Since
intermediate kernel buffer does not exist, I do not have a problem to
solve.

>My main concern, though, is forward and backward compatibility. Even
>when metadata is enabled, there are IO commands that don't touch it, so
>some tool that erroneously requested it will stop working. Or perhaps
>some other future opcode will have some other metadata use that doesn't
>match up exactly with how read/write/compare/append use it. As much as
>I'd like to avoid bad user commands from crashing, these kinds of checks
>can become problematic for maintenance. 

For forward compatibility - if we have commands that need to specify
metadata in a different way (than what is possible from this interface),
we anyway need a new passthrough command structure.
Moreover, it's really about caring _only_ for cases when kernel allocates
memory for metadata. And those cases are specific (i.e., when
metadata and metalen are not zero). We don't have to think in terms of
opcode (existing or future), no?

For backward comptability, should we care about erroneous tools.
My concern is we currenly have a hole that can be exploited to affect
other sane applications and bring the kernel down to its knees.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/2] nvme: fix memory corruption for passthrough metadata

[Keith Busch]

On Tue, Sep 05, 2023 at 10:48:25AM +0530, Kanchan Joshi wrote:
> On Fri, Sep 01, 2023 at 10:45:50AM -0400, Keith Busch wrote:
> > And similiar to this problem, what if the metadata is extended rather
> > than separate, and the user's buffer is too short? That will lead to the
> > same type of problem you're trying to fix here?
> 
> No.
> For extended metadata, userspace is using its own buffer. Since
> intermediate kernel buffer does not exist, I do not have a problem to
> solve.

We still use kernel memory if the user buffer is unaligned. If the user
space provides an short unaligned buffer, the device will corrupt kernel
memory.
 
> > My main concern, though, is forward and backward compatibility. Even
> > when metadata is enabled, there are IO commands that don't touch it, so
> > some tool that erroneously requested it will stop working. Or perhaps
> > some other future opcode will have some other metadata use that doesn't
> > match up exactly with how read/write/compare/append use it. As much as
> > I'd like to avoid bad user commands from crashing, these kinds of checks
> > can become problematic for maintenance.
> 
> For forward compatibility - if we have commands that need to specify
> metadata in a different way (than what is possible from this interface),
> we anyway need a new passthrough command structure.

Not sure about that. The existing struct is flexible enough to describe
any possible nvme command.

More specifically about compatibility is that this patch assumes an
"nlb" field exists inside an opaque structure at DW12 offset, and that
field defines how large the metadata buffer needs to be. Some vendor
specific or future opcode may have DW12 mean something completely
different, but still need to access metadata this patch may prevent from
working.

> Moreover, it's really about caring _only_ for cases when kernel
> allocates
> memory for metadata. And those cases are specific (i.e., when
> metadata and metalen are not zero). We don't have to think in terms of
> opcode (existing or future), no?

It looks like a little work, but I don't see why blk-integrity must use
kernel memory. Introducing an API like 'bio_integrity_map_user()' might
also address your concern, as long as the user buffer is aligned. It
sounds like we're assuming user buffers are aligned, at least.

Re: [PATCH v2 1/2] nvme: fix memory corruption for passthrough metadata

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 7171 bytes --]

On Tue, Sep 05, 2023 at 12:08:40PM -0600, Keith Busch wrote:
>On Tue, Sep 05, 2023 at 10:48:25AM +0530, Kanchan Joshi wrote:
>> On Fri, Sep 01, 2023 at 10:45:50AM -0400, Keith Busch wrote:
>> > And similiar to this problem, what if the metadata is extended rather
>> > than separate, and the user's buffer is too short? That will lead to the
>> > same type of problem you're trying to fix here?
>>
>> No.
>> For extended metadata, userspace is using its own buffer. Since
>> intermediate kernel buffer does not exist, I do not have a problem to
>> solve.
>
>We still use kernel memory if the user buffer is unaligned. If the user
>space provides an short unaligned buffer, the device will corrupt kernel
>memory.

Ah yes. blk_rq_map_user_iov() does make a copy of user-buffer in that
case.

>> > My main concern, though, is forward and backward compatibility. Even
>> > when metadata is enabled, there are IO commands that don't touch it, so
>> > some tool that erroneously requested it will stop working. Or perhaps
>> > some other future opcode will have some other metadata use that doesn't
>> > match up exactly with how read/write/compare/append use it. As much as
>> > I'd like to avoid bad user commands from crashing, these kinds of checks
>> > can become problematic for maintenance.
>>
>> For forward compatibility - if we have commands that need to specify
>> metadata in a different way (than what is possible from this interface),
>> we anyway need a new passthrough command structure.
>
>Not sure about that. The existing struct is flexible enough to describe
>any possible nvme command.
>
>More specifically about compatibility is that this patch assumes an
>"nlb" field exists inside an opaque structure at DW12 offset, and that
>field defines how large the metadata buffer needs to be. Some vendor
>specific or future opcode may have DW12 mean something completely
>different, but still need to access metadata this patch may prevent from
>working.

Right. It almost had me dropping the effort.
But given the horrible bug at hand, added an untested patch [1] that
handles all the shortcomings you mentioned. Please take a look.

>> Moreover, it's really about caring _only_ for cases when kernel
>> allocates
>> memory for metadata. And those cases are specific (i.e., when
>> metadata and metalen are not zero). We don't have to think in terms of
>> opcode (existing or future), no?
>
>It looks like a little work, but I don't see why blk-integrity must use
>kernel memory. Introducing an API like 'bio_integrity_map_user()' might
>also address your concern, as long as the user buffer is aligned. It
>sounds like we're assuming user buffers are aligned, at least.

Would you really prefer to have nvme_add_user_metadata() changed to do
away with allocation and use userspace meta-buffer directly?
Even with that route, extended-lba-with-short-unaligned-buffer remains 
unhandled. That will still require similar checks that I would like
to avoid but cannnot.

So how about this -

[1]
diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index d8ff796fd5f2..d09b5691da3e 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -320,6 +320,67 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
                        meta_len, lower_32_bits(io.slba), NULL, 0, 0);
 }

+static inline bool nvme_nlb_in_cdw12(u8 opcode)
+{
+       switch(opcode) {
+               case nvme_cmd_read:
+               case nvme_cmd_write:
+               case nvme_cmd_compare:
+               case nvme_cmd_zone_append:
+                       return true;
+       }
+       return false;
+}
+
+static bool nvme_validate_passthru_meta(struct nvme_ctrl *ctrl,
+                                       struct nvme_ns *ns,
+                                       struct nvme_command *c,
+                                       __u64 meta, __u32 meta_len,
+                                       unsigned data_len)
+{
+       /*
+        * User may specify smaller meta-buffer with a larger data-buffer.
+        * Driver allocated meta buffer will also be small.
+        * Device can do larger dma into that, overwriting unrelated kernel
+        * memory.
+        */
+       if (ns && (meta_len || meta || ns->features & NVME_NS_EXT_LBAS)) {
+               u16 nlb, control;
+               unsigned dlen, mlen;
+
+               /* Exclude commands that do not have nlb in cdw12 */
+               if (!nvme_nlb_in_cdw12(c->common.opcode))
+                       return true;
+
+               control = upper_16_bits(le32_to_cpu(c->common.cdw12));
+               /* Exclude when meta transfer from/to host is not done */
+               if (control & NVME_RW_PRINFO_PRACT && ns->ms == ns->pi_size)
+                       return true;
+
+               nlb = lower_16_bits(le32_to_cpu(c->common.cdw12));
+               mlen = (nlb + 1) * ns->ms;
+
+               /* sanity for interleaved buffer */
+               if (ns->features & NVME_NS_EXT_LBAS) {
+                       dlen = (nlb + 1) << ns->lba_shift;
+                       if (data_len < (dlen + mlen))
+                               goto out_false;
+                       return true;
+               }
+               /* sanity for separate meta buffer */
+               if (meta_len < mlen)
+                       goto out_false;
+
+               return true;
+out_false:
+               dev_err(ctrl->device,
+                       "%s: metadata length is small!\n", current->comm);
+               return false;
+       }
+
+       return true;
+}
+
 static bool nvme_validate_passthru_nsid(struct nvme_ctrl *ctrl,
                                        struct nvme_ns *ns, __u32 nsid)
 {
@@ -364,6 +425,10 @@ 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_validate_passthru_meta(ctrl, ns, &c, cmd.metadata,
+                                        cmd.metadata_len, cmd.data_len))
+               return -EINVAL;
+
        if (!nvme_cmd_allowed(ns, &c, 0, open_for_write))
                return -EACCES;

@@ -411,6 +476,10 @@ 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_validate_passthru_meta(ctrl, ns, &c, cmd.metadata,
+                                        cmd.metadata_len, cmd.data_len))
+               return -EINVAL;
+
        if (!nvme_cmd_allowed(ns, &c, flags, open_for_write))
                return -EACCES;

@@ -593,6 +662,10 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
        d.metadata_len = READ_ONCE(cmd->metadata_len);
        d.timeout_ms = READ_ONCE(cmd->timeout_ms);

+       if (!nvme_validate_passthru_meta(ctrl, ns, &c, d.metadata,
+                                        d.metadata_len, d.data_len))
+               return -EINVAL;
+
        if (issue_flags & IO_URING_F_NONBLOCK) {
                rq_flags |= REQ_NOWAIT;
                blk_flags = BLK_MQ_REQ_NOWAIT;
--
2.25.1

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/2] nvme: fix memory corruption for passthrough metadata

[Keith Busch]

On Wed, Sep 06, 2023 at 09:18:15PM +0530, Kanchan Joshi wrote:
> Would you really prefer to have nvme_add_user_metadata() changed to do
> away with allocation and use userspace meta-buffer directly?

I mean, sure, if it's possible. We can avoid a costly copy if the user
metabuffer is aligned and physically contiguous.

> Even with that route, extended-lba-with-short-unaligned-buffer remains
> unhandled. That will still require similar checks that I would like
> to avoid but cannnot.
> 
> So how about this -

There's lots of bad things you can do with this interface. Example,
provide an unaligned single byte user buffer and send an Identify
command.

We never provided opcode decoding sanity checks before because it's a
bad maintenance burden, adds performance killing overhead, couldn't
catch all the cases anyway due to vendor specific and future opcodes,
and harms the flexibility of the interface. The burden is usually on the
user for these kinds of priviledged interfaces: if you abuse it, "you
get to keep both pieces" territory.

Re: [PATCH v2 1/2] nvme: fix memory corruption for passthrough metadata

[Kanchan Joshi]

On Thu, Sep 7, 2023 at 9:11 PM Keith Busch <kbusch@kernel.org> wrote:
>
> On Wed, Sep 06, 2023 at 09:18:15PM +0530, Kanchan Joshi wrote:
> > Would you really prefer to have nvme_add_user_metadata() changed to do
> > away with allocation and use userspace meta-buffer directly?
>
> I mean, sure, if it's possible. We can avoid a costly copy if the user
> metabuffer is aligned and physically contiguous.

Seems possible, but that does not really solve the actual problem
(which is not performance) this patch is for.
It will require replicating big code of blk_rq_map_user_iov() for
integrity metadata and map the pages into bip.
But since the user-space meta buffer can be unaligned (and bunch of
other conditions present there), it has to make the meta copy in
kernel-space.
And we will be back to where we started - how to avoid corruption into
kernel memory.

Same situation for the case when we are dealing with
extended-lba-format and interleaved user-buffer is unaligned. .

Handling both these anyway requires adding the kind of code/checks
mentioned in the previous patch.
Do you see another way?

While I agree there is value in avoiding the meta copy in general and
I can look into it, but that should be a separate effort (with focus
on performance).

> > Even with that route, extended-lba-with-short-unaligned-buffer remains
> > unhandled. That will still require similar checks that I would like
> > to avoid but cannnot.
> >
> > So how about this -
>
> There's lots of bad things you can do with this interface. Example,
> provide an unaligned single byte user buffer and send an Identify
> command.

Not sure I follow. Do you mean the patch does not handle these cases?

> We never provided opcode decoding sanity checks before because it's a
> bad maintenance burden, adds performance killing overhead, couldn't
> catch all the cases anyway due to vendor specific and future opcodes,
> and harms the flexibility of the interface.

Given the way things are (integrity schemes, cdw12 etc.), I do not see
a way to avoid opcode checks.
Flexibility is not getting reduced in the previous patch. All the
other commands (beyond read/write/compare/append) remain untouched.
And metadata-io is not the fast path at the moment (given memory
allocation, bunch of extra things by blk-integrity, bunch of extra
things done by device etc.).

>The burden is usually on the
> user for these kinds of priviledged interfaces: if you abuse it, "you
> get to keep both pieces" territory.

Not sure I got that. Have you seen the crash mentioned in this cover
letter: https://lore.kernel.org/linux-nvme/20230811155906.15883-1-joshi.k@samsung.com/
A simple unprivileged read by a rogue application can wreck other
applications/system at the moment.
Is it fine to keep the status quo?