[PATCH 0/2] nvmet: fix configfs attribute validation

[PATCH 0/2] nvmet: fix configfs attribute validation

[shao.mingyin]

This series fixes two independent configfs attribute bugs in nvmet.

Patch 1 fixes a use-after-free when attr_tbkas is written while a
controller is going away.  The store callback walked subsys->ctrls under
nvmet_config_sem only, while the list is protected by subsys->lock and
nvmet_ctrl_free() does not take nvmet_config_sem.  Take subsys->lock for
the walk, as the qid_max store callback already does.

Patch 2 fixes an off-by-one write in the traddr and trsvcid store
callbacks.  A write of exactly NVMF_TRADDR_SIZE or NVMF_TRSVCID_SIZE
characters was accepted, and the following sscanf("%s") had no field
width, so it wrote the value plus the trailing NUL one byte past the
destination array.  Bound the scans to the array size.

Both patches are independent and can be applied in any order.  Both
carry Fixes: and Cc: stable.

Testing: both apply to next-20261030, pass checkpatch.pl --strict with
0 errors/0 warnings/0 checks, and drivers/nvme/target/configfs.c builds
with CONFIG_NVME_TARGET=m and W=1 without new warnings.

Patch 1 was reproduced with a KASAN kernel: hammering attr_tbkas while
loop controllers were continuously connected and deleted crashed the
unpatched kernel with a general protection fault in
nvmet_subsys_attr_tbkas_store() (KASAN wild-memory-access at a poisoned
list pointer).  With the patch the same 60-second stress, about 39k
iterations of the store, completed without any report.

Patch 2 was also reproduced and re-tested in QEMU (7.3.0-next-20261030):
a 256-character addr_traddr write cleared tsas.rdma.qptype (addr_tsas
went from "connected" to empty) before the fix, and is truncated to 255
characters with TSAS intact after it.

Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
base-commit: 7649ba73f9e7e1e04d0b46213e39c79ae09dd55c

[PATCH 1/2] nvmet: take subsys->lock when forcing reconnects for tbkas

[shao.mingyin]

nvmet_subsys_attr_tbkas_store() walks subsys->ctrls to force a reconnect
while holding only nvmet_config_sem.  The list is protected by
subsys->lock: nvmet_alloc_ctrl() adds entries and nvmet_ctrl_free()
removes them under that lock, and the controller free path does not take
nvmet_config_sem.  Writing attr_tbkas concurrently with a controller
going away, for example on a keep-alive timeout or a fatal error, can
therefore dereference a controller that has already been removed and
freed.

Take subsys->lock around the list walk, as
nvmet_subsys_attr_qid_max_store() already does for the same loop.
delete_ctrl() only schedules the asynchronous teardown, so it does not
recurse into subsys->lock.

Fixes: e98e45e9465b ("nvmet: Add traffic based keep alive support to configfs")
Cc: stable@vger.kernel.org
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
Tested on 7.3.0-next-20261030 (QEMU/TCG guest, KASAN) with the loop
transport.

One process hammers the attr_tbkas store while another connects and
deletes loop controllers, so the walk over subsys->ctrls races with
nvmet_ctrl_free():

  unpatched                            patched
  general protection fault,            no report in 60 s,
  KASAN wild-memory-access 0xdead...   39591 iterations
  in nvmet_subsys_attr_tbkas_store

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index b3a5242d06f9..c17f2c8ab2df 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1466,8 +1466,10 @@ static ssize_t nvmet_subsys_attr_tbkas_store(struct config_item *item,
 	subsys->tbkas = tbkas;
 
 	/* Force reconnect */
+	mutex_lock(&subsys->lock);
 	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry)
 		ctrl->ops->delete_ctrl(ctrl);
+	mutex_unlock(&subsys->lock);
 	up_write(&nvmet_config_sem);
 
 	return count;

Re: [PATCH 1/2] nvmet: take subsys->lock when forcing reconnects for tbkas

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/2] nvmet: bound the traddr and trsvcid configfs writes

[shao.mingyin]

nvmet_addr_traddr_store() and nvmet_addr_trsvcid_store() reject a write
only when count is larger than NVMF_TRADDR_SIZE (256) respectively
NVMF_TRSVCID_SIZE (32).  A write of exactly that many characters is
accepted, and the following

	sscanf(page, "%s\n", port->disc_addr.traddr)

has no field width.  configfs terminates the buffer at page[count], so
the scan writes 256 characters plus the trailing NUL, i.e. 257 bytes,
into traddr[256].  The extra byte lands in the following tsas field of
the discovery log entry, silently clearing the first TSAS byte (the
RDMA queue type, or the TLS section type for TCP).  trsvcid has the
same off-by-one with a 32-byte write; there the extra byte lands in
the reserved bytes that follow trsvcid.

Limit the scans to one byte less than the destination arrays.  The
count checks can stay as they are so that a normal newline-terminated
write is still accepted.

Fixes: 9ba2a5cb8896 ("nvmet: filter newlines from user input")
Cc: stable@vger.kernel.org
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
Tested on 7.3.0-next-20261030 (QEMU/TCG guest) with configfs; no RDMA
device is needed to set the port attributes.

Writing exactly NVMF_TRADDR_SIZE (256) characters to addr_traddr makes
the unterminated sscanf() store 257 bytes, so the trailing NUL clears
the first TSAS byte of the discovery entry:

  unpatched                            patched
  addr_tsas 'connected' -> ''          addr_tsas stays 'connected'
  addr_traddr read returns 257 bytes   read returns 256 bytes

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index b3a5242d06f9..8a6968b014c6 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -150,7 +150,7 @@ static ssize_t nvmet_addr_traddr_store(struct config_item *item,
 	if (nvmet_is_port_enabled(port, __func__))
 		return -EACCES;
 
-	if (sscanf(page, "%s\n", port->disc_addr.traddr) != 1)
+	if (sscanf(page, "%255s", port->disc_addr.traddr) != 1)
 		return -EINVAL;
 	return count;
 }
@@ -242,7 +242,7 @@ static ssize_t nvmet_addr_trsvcid_store(struct config_item *item,
 	if (nvmet_is_port_enabled(port, __func__))
 		return -EACCES;
 
-	if (sscanf(page, "%s\n", port->disc_addr.trsvcid) != 1)
+	if (sscanf(page, "%31s", port->disc_addr.trsvcid) != 1)
 		return -EINVAL;
 	return count;
 }

Re: [PATCH 2/2] nvmet: bound the traddr and trsvcid configfs writes

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>