linux-kernel - [PATCH] nvmet-auth: fix length calculation in nvmet_auth

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <2abae353-5e30-4dc5-a2cd-26dab4db93d0@stanley.mountain>
Date: Wed, 8 Jan 2025 12:34:04 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Hannes Reinecke <hare@...e.de>
Cc: Christoph Hellwig <hch@....de>, Sagi Grimberg <sagi@...mberg.me>,
	Chaitanya Kulkarni <kch@...dia.com>, Jens Axboe <axboe@...nel.dk>,
	linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org,
	kernel-janitors@...r.kernel.org
Subject: [PATCH] nvmet-auth: fix length calculation in nvmet_auth_challenge()

The "d" variable is a void pointer so sizeof(*d) is 1.  It was supposed
to be sizeof(*data) which is 16.

The "data_size" is the data required to hold the data struct plus
"hash_len" which is the length of the variable array at the end of the
data struct.  Plus the "ctrl->dh_keysize" which is the extra space after
the end of the data struct.  The "al" variable is actual length of the
buffer.

This mistake means that we will not zero the last 15 bytes.  We likely
copy data over these bytes so it may not be an issue.  The main problem
is that the check "if (al < data_size)" which ensures that we have
allocated enough data is incorrect, potentially leading to memory
corruption.

Cc: stable@...r.kernel.org
Fixes: db1312dd9548 ("nvmet: implement basic In-Band Authentication")
Signed-off-by: Dan Carpenter <dan.carpenter@...aro.org>
---
I thought about changing the caller to use kzalloc() instead of kmalloc()
to get rid of the memset().  But we need to calculate data_size anyway
so moving the memset() doesn't really add very much.

 drivers/nvme/target/fabrics-cmd-auth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
index 3f2857c17d95..aad113e17072 100644
--- a/drivers/nvme/target/fabrics-cmd-auth.c
+++ b/drivers/nvme/target/fabrics-cmd-auth.c
@@ -356,7 +356,7 @@ static int nvmet_auth_challenge(struct nvmet_req *req, void *d, int al)
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
 	int ret = 0;
 	int hash_len = nvme_auth_hmac_hash_len(ctrl->shash_id);
-	int data_size = sizeof(*d) + hash_len;
+	int data_size = sizeof(*data) + hash_len;

 	if (ctrl->dh_tfm)
 		data_size += ctrl->dh_keysize;
-- 
2.45.2