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

[Keith Busch <kbusch@kernel.org>]

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.