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

Alon Zahavi zahavi.alon at gmail.com
Wed Nov 8 03:02:38 PST 2023


On Wed, 8 Nov 2023 at 12:02, Chaitanya Kulkarni <chaitanyak at nvidia.com> wrote:
>
> 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
>
>

I think that although those patches will fix the bug in the two
functions we talked about, the non-terminated strings are still stored
in the `struct nvmf_connect_data` object.
This can cause major security bugs in the future. Moreover, handling
this kind of copy of strings before each time we use `subsysnqn` or
`sysnqn` is taking resources and time.
For example, in `nvmf_connect_data_prep()` it uses
`strncpy(data->subsysnqn, ctrl->opts->subsysnqn, NVMF_NQN_SIZE);` to
copy the subsysnqn to the object, with NVMF_NQN_SIZE as the max size.

Is it possible to add a NULL terminator after each time we use
`nvmet_copy_from_sgl()` to copy from an SGL to `struct
nvmf_connect_data` object?
Also, I think `nvmet_copy_from_sgl()` should be considered as unsafe
when using it to copy strings, as it doesn't check for NULL
termination.

Thanks,
Alon.



More information about the Linux-nvme mailing list