Re: [PATCH] nvme: disallow passthru cmd from targeting a nsid != nsid of the block dev

[Niklas Cassel]

On Fri, Mar 26, 2021 at 12:19:42AM +0900, Keith Busch wrote:
> On Thu, Mar 25, 2021 at 09:48:37AM +0000, Niklas Cassel wrote:
> > From: Niklas Cassel <niklas.cassel@xxxxxxx>
> > 
> > When a passthru command targets a specific namespace, the ns parameter to
> > nvme_user_cmd()/nvme_user_cmd64() is set. However, there is currently no
> > validation that the nsid specified in the passthru command targets the
> > namespace/nsid represented by the block device that the ioctl was
> > performed on.
> > 
> > Add a check that validates that the nsid in the passthru command matches
> > that of the supplied namespace.
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
> > ---
> > Currently, if doing NVME_IOCTL_IO_CMD on the controller char device,
> > if and only if there is a single namespace in the ctrl->namespaces list,
> > nvme_dev_user_cmd() will call nvme_user_cmd() with ns parameter set.
> > While it might be good that we validate the nsid even in this case,
> > perhaps we want to allow a nsid value in the passthru command to be
> > 0x0 and/or the NSID broadcast value? (Only when NVME_IOCTL_IO_CMD was
> > done on the controller char device though.)
> 
> There are no IO commands accepting a 0 NSID, so rejecting those from the
> driver should be okay.
> 
> FLUSH is the only IO command that takes a broadcast NSID. I suspect at
> least some entities were unfortunately sending broadcast flush through
> this interface, so it's possible we'll hear of breakage, but I'd agree
> your patch is still the right thing to do.

I don't think this should be a problem.

You shouldn't be sending a broadcast NSID via the per namespace block
device. It just seems silly to specify a specific namespace block device,
and then use the broadcast NSID. (This obviously should never have been
allowed.)

If you wanted to send a broadcast NSID, you should have used the controller
character device. However, nvme_dev_user_cmd() currently rejects any
NVME_IOCTL_IO_CMD when there is more than one namespace in ctrl->namespaces
list, so they could never have used the controller character device to send
a flush to more than one namespace. So here we don't change anything.


Kind regards,
Niklas