[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