Re: [PATCH v2 03/14] nvmet: Implement CCR nvme command

[Randy Jennings]

On Mon, Feb 9, 2026 at 11:27 AM Mohamed Khalfella
<mkhalfella@xxxxxxxxxxxxxxx> wrote:
>
> On Sun 2026-02-08 15:10:36 -0800, Mohamed Khalfella wrote:
> > On Sat 2026-02-07 15:58:49 +0200, Sagi Grimberg wrote:
> > >
> > >
> > > On 04/02/2026 19:52, Mohamed Khalfella wrote:
> > > > On Wed 2026-02-04 01:55:18 +0100, Hannes Reinecke wrote:
> > > >> On 2/4/26 01:44, Mohamed Khalfella wrote:
> > > >>> On Wed 2026-02-04 01:38:44 +0100, Hannes Reinecke wrote:
> > > >>>> On 2/3/26 19:40, Mohamed Khalfella wrote:
> > > >>>>> On Tue 2026-02-03 04:19:50 +0100, Hannes Reinecke wrote:
> > > >>>>>> On 1/30/26 23:34, Mohamed Khalfella wrote:
> > > >>>>>>> @@ -1501,6 +1516,38 @@ struct nvmet_ctrl *nvmet_ctrl_find_get(const char *subsysnqn,
> > > >>>>>>>             return ctrl;
> > > >>>>>>>      }
> > > >>>>>>>
> > > >>>>>>> +struct nvmet_ctrl *nvmet_ctrl_find_get_ccr(struct nvmet_subsys *subsys,
> > > >>>>>>> +                                      const char *hostnqn, u8 ciu,
> > > >>>>>>> +                                      u16 cntlid, u64 cirn)
> > > >>>>>>> +{
> > > >>>>>>> +   struct nvmet_ctrl *ctrl;
> > > >>>>>>> +   bool found = false;
> > > >>>>>>> +
> > > >>>>>>> +   mutex_lock(&subsys->lock);
> > > >>>>>>> +   list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
> > > >>>>>>> +           if (ctrl->cntlid != cntlid)
> > > >>>>>>> +                   continue;
> > > >>>>>>> +           if (strncmp(ctrl->hostnqn, hostnqn, NVMF_NQN_SIZE))
> > > >>>>>>> +                   continue;
> > > >>>>>>> +
> > > >>>>>> Why do we compare the hostnqn here, too? To my understanding the host
> > > >>>>>> NQN is tied to the controller, so the controller ID should be sufficient
> > > >>>>>> here.
> > > >>>>> We got cntlid from CCR nvme command and we do not trust the value sent by
> > > >>>>> the host. We check hostnqn to confirm that host is actually connected to
> > > >>>>> the impacted controller. A host should not be allowed to reset a
> > > >>>>> controller connected to another host.
> > > >>>>>
> > > >>>> Errm. So we're starting to not trust values in NVMe commands?
> > > >>>> That is a very slippery road.
> > > >>>> Ultimately it would require us to validate the cntlid on each
> > > >>>> admin command. Which we don't.
> > > >>>> And really there is no difference between CCR and any other
> > > >>>> admin command; you get even worse effects if you would assume
> > > >>>> a misdirected 'FORMAT' command.
> > > >>>>
> > > >>>> Please don't. Security is _not_ a concern here.
> > > >>> I do not think the check hurts. If you say it is wrong I will delete it.
> > > >>>
> > > >> It's not 'wrong', It's inconsistent. The argument that the contents of
> > > >> an admin command may be wrong applies to _every_ admin command.
> > > >> Yet we never check on any of those commands.
> > > >> So I fail to see why this command requires special treatment.
> > > > Okay, I will delete this check.
Security is a concern.  But not to validate the ctrlId per se.

CCR is a different type of command from other NVMe commands
because it affects a different controller, one that could be connected
to a different host.

The Host NQN check was put in specifically to prevent one host from
aborting connections on another host.  One host terminating the
association with another host is definitely an attack vector, and it was
considered when the command was designed.

Authentication in NVMe is tied around the NQN.  Yes, another host
could fake its NQN, and authentication is supposed to catch that.  Of
course you have to implement authentication and have it turned on.

Hannas, why do you think security is not a concern?  Because
authentication is not implemented?  I can buy that; a comment
that says "Not verifying host nqn because authentication is not
implemented either." would work.

> > >
> > > It is a very different command than other commands that nvmet serves. Format
> > > is different because it is an attached namespace, hence the host should
> > > be able
> > > to format it. If it would have been possible to access a namespace that
> > > is not mapped
> > > to a controller, then this check would have been warranted I think.
> > >
> > > There have been some issues lately opened on nvme-tcp that expose
> > > attacks that can
> > > crash the kernel with some hand-crafted commands, I'd say that this is a
> > > potential attack vector.
The two NQNs being compared comes from the ctrl structures, which have
already had their NQNs logged on connect.  What attack vector are you
seeing here with the NQNs?

> >
> > For an attacker to exploit CCR command they will have to guess both CUI
> > (8bit) and CIRN(64bit) random values correctly. I do not see how an
> > attacker can find these values without being connected to the impacted
> > controller.
>
> I changed ciu and cirn sysfs controller attributes on the host from S_IRUGO
> to S_IRUSR. This should mitigate an attack by unprivileged process
> running on host. On the target side debugfs files already have S_IRUSR
> so no change is needed.
>

Sincerely,
Randy Jennings