[PATCH] scsi: ses: unregister the enclosure before freeing its device state

[PATCH] scsi: ses: unregister the enclosure before freeing its device state

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

ses_intf_remove_enclosure() frees ses_dev and the page1/page2/page10
buffers it owns before calling enclosure_unregister().  Only
enclosure_unregister() tears down the component sysfs attributes -- which
drains any in-flight get or set access -- and repoints edev->cb at the null
callbacks, so between the frees and that call the attributes stay live over
freed memory.  A concurrent read or write of a component attribute then
dereferences the freed edev->scratch in ses_get_page2_descriptor() or
ses_show_id(): a use-after-free reachable while the enclosure is removed
(hot unplug or delete) and its sysfs is accessed.  The early
edev->scratch = NULL does not help -- ses_page2_supported() does not check
it, so it only turns the use-after-free into a NULL dereference.

Unregister first, then free.  Save the component scratch pointer while edev
is still alive (enclosure_unregister() drops the enclosure device), and
drop the now-redundant early scratch clear.

Fixes: 9927c68864e9 ("[SCSI] ses: add new Enclosure ULD")
Closes: https://sashiko.dev/#/patchset/20260706-b4-disp-29a05ca3-v1-1-49591f469f60@proton.me?part=1
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Reproduced with an in-kernel KASAN litmus modelling the removal ordering
and the sysfs accessor: freeing ses_dev before enclosure_unregister() and
then dereferencing the freed scratch reports slab-use-after-free in
ses_get_page2_descriptor() (the page2 pointer, 16 bytes into the kmalloc-64
ses_device) and in ses_show_id() (the page1 pointer, offset 0); reordering
so enclosure_unregister() runs first is clean, as is the single-threaded
case.  Because the freed ses_device is a kmalloc-64 pointer carrier, a
reclaim of the freed slot turns ses_show_id()'s page1 read into an
arbitrary-read oracle and ses_get_page2_descriptor() -> ses_recv_diag()
into an arbitrary write.

Triggerable while a component sysfs attribute (fault/status/locate/active/
id) is read or written concurrently with enclosure removal (hot unplug or a
delete); no malicious device required.
---
 drivers/scsi/ses.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 4c348645b04e..a3039a3ede56 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -862,6 +862,7 @@ static void ses_intf_remove_enclosure(struct scsi_device *sdev)
 {
 	struct enclosure_device *edev;
 	struct ses_device *ses_dev;
+	void *scomp;
 
 	/*  exact match to this enclosure */
 	edev = enclosure_find(&sdev->sdev_gendev, NULL);
@@ -869,18 +870,24 @@ static void ses_intf_remove_enclosure(struct scsi_device *sdev)
 		return;
 
 	ses_dev = edev->scratch;
-	edev->scratch = NULL;
+	scomp = edev->components ? edev->component[0].scratch : NULL;
+
+	/*
+	 * Unregister before freeing.  enclosure_unregister() tears down the
+	 * component sysfs attributes, draining any in-flight get or set access,
+	 * and points edev->cb at the null callbacks.  Freeing ses_dev and the
+	 * pages it references first leaves those attributes live over freed
+	 * memory, so a concurrent component-attribute access dereferences the
+	 * freed edev->scratch in ses_get_page2_descriptor() or ses_show_id().
+	 */
+	put_device(&edev->edev);
+	enclosure_unregister(edev);
 
 	kfree(ses_dev->page10);
 	kfree(ses_dev->page1);
 	kfree(ses_dev->page2);
 	kfree(ses_dev);
-
-	if (edev->components)
-		kfree(edev->component[0].scratch);
-
-	put_device(&edev->edev);
-	enclosure_unregister(edev);
+	kfree(scomp);
 }
 
 static void ses_intf_remove(struct device *cdev)

---
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
change-id: 20260706-b4-disp-cf600574-2a11b71951aa

Best regards,
-- 
Bryam Vargas <hexlabsecurity@proton.me>

Re: [PATCH] scsi: ses: unregister the enclosure before freeing its device state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The enclosure device is published to sysfs and the global list before its private data pointers are initialized, allowing concurrent accesses to trigger a NULL pointer dereference.
--

commit c0db7f74419376e5dfdb039e9a143c3b6a8d01bf
Author: Bryam Vargas <hexlabsecurity@proton.me>

scsi: ses: unregister the enclosure before freeing its device state

ses_intf_remove_enclosure() frees ses_dev and the page1/page2/page10
buffers it owns before calling enclosure_unregister(). This leaves the
component sysfs attributes live over freed memory, meaning a concurrent
read or write could trigger a use-after-free. This patch reorders the
teardown to unregister first, then free.

Fixes: 9927c68864e9 ("[SCSI] ses: add new Enclosure ULD")
Closes: https://sashiko.dev/#/patchset/20260706-b4-disp-29a05ca3-v1-1-49591f469f60@proton.me?part=1
Cc: stable@vger.kernel.org

> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> index 4c348645b04ee..a3039a3ede56c 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -869,18 +870,24 @@ static void ses_intf_remove_enclosure(struct scsi_device *sdev)
>  		return;
>  
>  	ses_dev = edev->scratch;
> -	edev->scratch = NULL;

[Severity: High]
This is a pre-existing issue, but since we are looking at sysfs race
conditions involving edev->scratch, could a similar race occur during
initialization in ses_intf_add()?

When ses_intf_add() registers the enclosure:

drivers/scsi/ses.c:ses_intf_add() {
	edev = enclosure_register(cdev->parent, dev_name(&sdev->sdev_gendev),
				  components, &ses_enclosure_callbacks);
	if (IS_ERR(edev)) {
		err = PTR_ERR(edev);
		goto err_free;
	}

	kfree(hdr_buf);

	edev->scratch = ses_dev;
	...
}

The enclosure_register() call makes the device visible to userspace and
could trigger a read of the id sysfs attribute before edev->scratch is
assigned.

If that happens, would the id_show callback lead to a NULL pointer
dereference here?

drivers/scsi/ses.c:ses_show_id() {
	struct ses_device *ses_dev = edev->scratch;
	unsigned long long id = get_unaligned_be64(ses_dev->page1+8+4);
	...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-b4-disp-cf600574-v1-1-be4f324ff44c@proton.me?part=1