Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: adM344tlMulK/QI/jMMj/Y50X9wwuUGFok8HYNn8MBFhmVrSWFDun+VB1N9B60KR/Tkg+1TbH5
 msxZT0AfkynW7PwgL6FaeWuXFAwVkdVWG1gOWv1cAJZpDcJGZT/94xtN8U+0FGS1Eh0S6THOSA
 b6Qt06SP4YaeXWcDk12Nfm49Ix5Hs66pfrLswwQTfrw6NVOWK7t73sPMOS42UcGYJdfHqmaNQ2
 uNYlaPeaz3CTMY6aQgLzecwwEvjJ0S2n+zAW8Ufyq3AjNpHetpuJb209B87EP9fdzU8bwOYP0U
 1QQ=
From:   Niklas Cassel <Niklas.Cassel@wdc.com>
To:     Keith Busch <kbusch@kernel.org>
CC:     Jens Axboe <axboe@fb.com>, Christoph Hellwig <hch@lst.de>,
        Sagi Grimberg <sagi@grimberg.me>,
        "minwoo.im.dev@gmail.com" <minwoo.im.dev@gmail.com>,
        "javier@javigon.com" <javier@javigon.com>,
        "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] nvme: disallow passthru cmd from targeting a nsid != nsid
 of the block dev
Thread-Topic: [PATCH] nvme: disallow passthru cmd from targeting a nsid !=
 nsid of the block dev
Thread-Index: AQHXIVwH1ScTxDZzaECPp8Ru2XC2tKqU0dcAgAHgdQA=
Date:   Fri, 26 Mar 2021 19:59:21 +0000
Message-ID: <YF49GFrXUyMkNNhA@x1-carbon.lan>
References: <20210325094807.328126-1-Niklas.Cassel@wdc.com>
 <20210325151942.GB31394@redsun51.ssa.fujisawa.hgst.com>
In-Reply-To: <20210325151942.GB31394@redsun51.ssa.fujisawa.hgst.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-ID: <010AD3AA47539140B6F9B1F9A1E9DEAB@namprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: ee64acdc-5fcd-4e1e-a722-08d8f091a613
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Mar 2021 19:59:21.8335
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: ga5A6eTEltaItIszq7/7gZ2MUkdYgi9rTM+rsFn7URPIV/pr2DRsKe9i5eASdTzpcoY+FcBUCUbaG3XWXtkUXA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR04MB5755
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@wdc.com>
> >=20
> > When a passthru command targets a specific namespace, the ns parameter =
to
> > nvme_user_cmd()/nvme_user_cmd64() is set. However, there is currently n=
o
> > validation that the nsid specified in the passthru command targets the
> > namespace/nsid represented by the block device that the ioctl was
> > performed on.
> >=20
> > Add a check that validates that the nsid in the passthru command matche=
s
> > that of the supplied namespace.
> >=20
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > ---
> > 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.)
>=20
> There are no IO commands accepting a 0 NSID, so rejecting those from the
> driver should be okay.
>=20
> 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=