[Bug Report] NVMe-oF/TCP - Slab OOB Read in `nvmet_ctrl_find_get`

[Chaitanya Kulkarni]

On 11/8/23 01:01, Alon Zahavi wrote:
> On Wed, 8 Nov 2023 at 10:46, Chaitanya Kulkarni <chaitanyak at nvidia.com> wrote:
>>
>>> ## Root Cause
>>> As explained above, the root cause for this bug is the fact that there
>>> are no NULL terminators to the strings in the object representing the
>>> `struct nvmf_connect_data`.
>> Can you see if this works for you ? it should at least take care of the
>> subsysnqn and hostnqn being accessed as NULL terminated string.
>>
>> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
>> index 3935165048e7..569046b6a269 100644
>> --- a/drivers/nvme/target/core.c
>> +++ b/drivers/nvme/target/core.c
>> @@ -1235,13 +1235,19 @@ struct nvmet_ctrl *nvmet_ctrl_find_get(const
>> char *subsysnqn,
>>                                          const char *hostnqn, u16 cntlid,
>>                                          struct nvmet_req *req)
>>    {
>> +       char subsysnqn_str[NVMF_NQN_SIZE + 1] = { 0 };
>> +       char hostnqn_str[NVMF_NQN_SIZE + 1] = { 0 };
>>           struct nvmet_ctrl *ctrl = NULL;
>>           struct nvmet_subsys *subsys;
>>
>> -       subsys = nvmet_find_get_subsys(req->port, subsysnqn);
>> +       /* subsysnqn & hostnqn may not be NULL ternimated */
>> +       strncpy(subsysnqn_str, subsysnqn, NVMF_NQN_SIZE);
>> +       strncpy(hostnqn_str, hostnqn, NVMF_NQN_SIZE);
>> +
>> +       subsys = nvmet_find_get_subsys(req->port, subsysnqn_str);
>>           if (!subsys) {
>>                   pr_warn("connect request for invalid subsystem %s!\n",
>> -                       subsysnqn);
>> +                       subsysnqn_str);
>>                   req->cqe->result.u32 = IPO_IATTR_CONNECT_DATA(subsysnqn);
>>                   goto out;
>>           }
>> @@ -1249,7 +1255,7 @@ struct nvmet_ctrl *nvmet_ctrl_find_get(const char
>> *subsysnqn,
>>           mutex_lock(&subsys->lock);
>>           list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
>>                   if (ctrl->cntlid == cntlid) {
>> -                       if (strncmp(hostnqn, ctrl->hostnqn,
>> NVMF_NQN_SIZE)) {
>> +                       if (strncmp(hostnqn_str, ctrl->hostnqn,
>> NVMF_NQN_SIZE)) {
>>                                   pr_warn("hostnqn mismatch.\n");
>>                                   continue;
>>                           }
>> @@ -1263,7 +1269,7 @@ struct nvmet_ctrl *nvmet_ctrl_find_get(const char
>> *subsysnqn,
>>
>>           ctrl = NULL; /* ctrl not found */
>>           pr_warn("could not find controller %d for subsys %s / host %s\n",
>> -               cntlid, subsysnqn, hostnqn);
>> +               cntlid, subsysnqn_str, hostnqn_str);
>>           req->cqe->result.u32 = IPO_IATTR_CONNECT_DATA(cntlid);
>>
>>    found:
>>
>>
>> -ck
>>
>>
> Before I check it I should say that the same bug occurs in other
> functions as well.
> For example in `nvmet_alloc_ctrl()` there is the same print.
>

yes it has same pattern and will need a same fix here is an updated
patch with nvmet_alloc_ctrl() fix :-


diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 3935165048e7..5c1a71d16df7 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1235,13 +1235,19 @@ struct nvmet_ctrl *nvmet_ctrl_find_get(const 
char *subsysnqn,
                                        const char *hostnqn, u16 cntlid,
                                        struct nvmet_req *req)
  {
+       char subsysnqn_str[NVMF_NQN_SIZE + 1] = { 0 };
+       char hostnqn_str[NVMF_NQN_SIZE + 1] = { 0 };
         struct nvmet_ctrl *ctrl = NULL;
         struct nvmet_subsys *subsys;

-       subsys = nvmet_find_get_subsys(req->port, subsysnqn);
+       /* subsysnqn & hostnqn may not be NULL ternimated */
+       strncpy(subsysnqn_str, subsysnqn, NVMF_NQN_SIZE);
+       strncpy(hostnqn_str, hostnqn, NVMF_NQN_SIZE);
+
+       subsys = nvmet_find_get_subsys(req->port, subsysnqn_str);
         if (!subsys) {
                 pr_warn("connect request for invalid subsystem %s!\n",
-                       subsysnqn);
+                       subsysnqn_str);
                 req->cqe->result.u32 = IPO_IATTR_CONNECT_DATA(subsysnqn);
                 goto out;
         }
@@ -1249,7 +1255,7 @@ struct nvmet_ctrl *nvmet_ctrl_find_get(const char 
*subsysnqn,
         mutex_lock(&subsys->lock);
         list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
                 if (ctrl->cntlid == cntlid) {
-                       if (strncmp(hostnqn, ctrl->hostnqn, 
NVMF_NQN_SIZE)) {
+                       if (strncmp(hostnqn_str, ctrl->hostnqn, 
NVMF_NQN_SIZE)) {
                                 pr_warn("hostnqn mismatch.\n");
                                 continue;
                         }
@@ -1263,7 +1269,7 @@ struct nvmet_ctrl *nvmet_ctrl_find_get(const char 
*subsysnqn,

         ctrl = NULL; /* ctrl not found */
         pr_warn("could not find controller %d for subsys %s / host %s\n",
-               cntlid, subsysnqn, hostnqn);
+               cntlid, subsysnqn_str, hostnqn_str);
         req->cqe->result.u32 = IPO_IATTR_CONNECT_DATA(cntlid);

  found:
@@ -1358,25 +1364,32 @@ static void nvmet_fatal_error_handler(struct 
work_struct *work)
  u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
                 struct nvmet_req *req, u32 kato, struct nvmet_ctrl **ctrlp)
  {
+       char subsysnqn_str[NVMF_NQN_SIZE + 1] = { 0 };
+       char hostnqn_str[NVMF_NQN_SIZE + 1] = { 0 };
         struct nvmet_subsys *subsys;
         struct nvmet_ctrl *ctrl;
         int ret;
         u16 status;

         status = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
-       subsys = nvmet_find_get_subsys(req->port, subsysnqn);
+
+       /* subsysnqn & hostnqn may not be NULL ternimated */
+       strncpy(subsysnqn_str, subsysnqn, NVMF_NQN_SIZE);
+       strncpy(hostnqn_str, hostnqn, NVMF_NQN_SIZE);
+
+       subsys = nvmet_find_get_subsys(req->port, subsysnqn_str);
         if (!subsys) {
                 pr_warn("connect request for invalid subsystem %s!\n",
-                       subsysnqn);
+                       subsysnqn_str);
                 req->cqe->result.u32 = IPO_IATTR_CONNECT_DATA(subsysnqn);
                 req->error_loc = offsetof(struct nvme_common_command, 
dptr);
                 goto out;
         }

         down_read(&nvmet_config_sem);
-       if (!nvmet_host_allowed(subsys, hostnqn)) {
+       if (!nvmet_host_allowed(subsys, hostnqn_str)) {
                 pr_info("connect by host %s for subsystem %s not 
allowed\n",
-                       hostnqn, subsysnqn);
+                       hostnqn_str, subsysnqn_str);
                 req->cqe->result.u32 = IPO_IATTR_CONNECT_DATA(hostnqn);
                 up_read(&nvmet_config_sem);
                 status = NVME_SC_CONNECT_INVALID_HOST | NVME_SC_DNR;

-ck