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

[Alon Zahavi]

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.

```
// From `linux/include/nvme.h`

/* NQN names in commands fields specified one size */
#define NVMF_NQN_FIELD_LEN 256

/* However the max length of a qualified name is another size */
#define NVMF_NQN_SIZE 223
```
>From the macros above we can see that the actual NVMF_MAX_SIZE is 223
while the field in the original object is 256.
Can we zero out the array from index 223 till the end of the array
during the initialization of `subsysnqn` and `hostnqn`?
This way we can mitigate future problems with the same object's fields.

Thanks,
Alon.