Commit 4b1aa5da authored by Deepak Abraham Tom's avatar Deepak Abraham Tom Committed by Jim Harris
Browse files

nvme: clear user_buffer pointer after use 



nvme_allocate_request() does not clear all the fields of the request structure.
SPDK generates internal admin commands that set user_buffer and related fields.
When the request structure associated with these commands are reused, the
user_buffer and related fields will contain stale data. If there is an error in
submitting this request, it will cause a bad free.

Change-Id: Ie60f5baad6f12dc1f42edd3116b9ae59ff51e67c
Signed-off-by: default avatarDeepak Abraham Tom <deepak-abraham.tom@hpe.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/20250


Reviewed-by: default avatarMichael Haeuptle <michaelhaeuptle@gmail.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarBen Walker <ben@nvidia.com>
Reviewed-by: default avatarJim Harris <jim.harris@samsung.com>
Reviewed-by: default avatarVasuki Manikarnike <vasuki.manikarnike@hpe.com>
Community-CI: Mellanox Build Bot
parent ee06b4c2

lib/nvme/nvme.c

+9 −4
Original line number Diff line number Diff line
@@ -393,10 +393,12 @@ static void 
nvme_user_copy_cmd_complete(void *arg, const struct spdk_nvme_cpl *cpl)

{

	struct nvme_request *req = arg;

	spdk_nvme_cmd_cb user_cb_fn;

	void *user_cb_arg;

	enum spdk_nvme_data_transfer xfer;



	if (req->user_buffer && req->payload_size) {

		/* Copy back to the user buffer and free the contig buffer */

		/* Copy back to the user buffer */

		assert(nvme_payload_type(&req->payload) == NVME_PAYLOAD_TYPE_CONTIG);

		xfer = spdk_nvme_opc_get_data_transfer(req->cmd.opc);

		if (xfer == SPDK_NVME_DATA_CONTROLLER_TO_HOST ||
@@ -404,12 +406,15 @@ nvme_user_copy_cmd_complete(void *arg, const struct spdk_nvme_cpl *cpl) 
			assert(req->pid == getpid());

			memcpy(req->user_buffer, req->payload.contig_or_cb_arg, req->payload_size);

		}



		spdk_free(req->payload.contig_or_cb_arg);

	}



	user_cb_fn = req->user_cb_fn;

	user_cb_arg = req->user_cb_arg;

	nvme_cleanup_user_req(req);



	/* Call the user's original callback now that the buffer has been copied */

	req->user_cb_fn(req->user_cb_arg, cpl);

	user_cb_fn(user_cb_arg, cpl);



}



/**

lib/nvme/nvme_ctrlr.c

+1 −3
Original line number Diff line number Diff line
@@ -3378,9 +3378,7 @@ nvme_ctrlr_cleanup_process(struct spdk_nvme_ctrlr_process *proc) 
		STAILQ_REMOVE(&proc->active_reqs, req, nvme_request, stailq);



		assert(req->pid == proc->pid);

		if (req->user_buffer && req->payload_size) {

			spdk_free(req->payload.contig_or_cb_arg);

		}

		nvme_cleanup_user_req(req);

		nvme_free_request(req);

	}

lib/nvme/nvme_internal.h

+12 −0
Original line number Diff line number Diff line
@@ -1409,6 +1409,18 @@ nvme_complete_request(spdk_nvme_cmd_cb cb_fn, void *cb_arg, struct spdk_nvme_qpa 
	}

}



static inline void

nvme_cleanup_user_req(struct nvme_request *req)

{

	if (req->user_buffer && req->payload_size) {

		spdk_free(req->payload.contig_or_cb_arg);

		req->user_buffer = NULL;

	}



	req->user_cb_arg = NULL;

	req->user_cb_fn = NULL;

}



static inline void

nvme_qpair_set_state(struct spdk_nvme_qpair *qpair, enum nvme_qpair_state state)

{

lib/nvme/nvme_pcie_common.c

+1 −3
Original line number Diff line number Diff line
@@ -291,9 +291,7 @@ nvme_pcie_qpair_insert_pending_admin_request(struct spdk_nvme_qpair *qpair, 
	} else {

		SPDK_ERRLOG("The owning process (pid %d) is not found. Dropping the request.\n",

			    active_req->pid);

		if (active_req->user_buffer && active_req->payload_size) {

			spdk_free(active_req->payload.contig_or_cb_arg);

		}

		nvme_cleanup_user_req(active_req);

		nvme_free_request(active_req);

	}

}

lib/nvme/nvme_qpair.c

+1 −4
Original line number Diff line number Diff line
@@ -1070,10 +1070,7 @@ error: 
		return rc;

	}



	if (req->user_buffer && req->payload_size) {

		spdk_free(req->payload.contig_or_cb_arg);

	}



	nvme_cleanup_user_req(req);

	nvme_free_request(req);



	return rc;