[PATCH v2 0/1] RDMA/srpt: Do not register event handler until srpt device is fully setup

[PATCH v2 0/1] RDMA/srpt: Do not register event handler until srpt device is fully setup

[William Kucharski]

Upon occasion, KASAN testing would report a use-after-free Write in
srpt_refresh_port().

In the course of trying to diagnose this, I noticed that the code in
srpt_add_one() registers an event handler for the srpt device and then
initializes the ports on the device. If any portion of the
device port initialization fails, it removes the registration for the
event handler in the error leg.

This felt like a race condition, where an event handler was registered
before the device ports were fully initialized.

While I can't definitively say this was the issue - this change may just
modify timing to mask the real issue - when modified to not register
the event handler until all of the device ports are intialized,
the issue no longer reproduces in KASAN.

Changelog:
v2:
  * Added Fixes tag

William Kucharski (1):
  RDMA/srpt: Do not register event handler until srpt device is fully setup

 drivers/infiniband/ulp/srpt/ib_srpt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

-- 
2.39.3


Upon occasion, KASAN testing would report a use-after-free Write in
srpt_refresh_port().

In the course of trying to diagnose this, I noticed that the code in
srpt_add_one() registers an event handler for the srpt device and then
initializes the ports on the device. If any portion of the
device port initialization fails, it removes the registration for the
event handler in the error leg.

This felt like a race condition, where an event handler was registered
before the device ports were fully initialized.

While I can't definitively say this was the issue - this change may just
modify timing to mask the real issue - when modified to not register
the event handler until all of the device ports are intialized,
the issue no longer reproduces in KASAN.

I'm submitting  this patch if only so those better acquainted with
the details of this procedure can analyze whether this was an actual
issue or just intellectual uncomfortableness with the code.

William Kucharski (1):
  Upon rare occasions, KASAN reports a use-after-free Write in srpt_refresh_port().

 drivers/infiniband/ulp/srpt/ib_srpt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

-- 
2.43.0

[PATCH v2 1/1] RDMA/srpt: Do not register event handler until srpt device is fully setup

[William Kucharski]

Upon rare occasions, KASAN reports a use-after-free Write
in srpt_refresh_port().

This seems to be because an event handler is registered before the
srpt device is fully setup and a race condition upon error may leave a
partially setup event handler in place.

Instead, only register the event handler after srpt device initialization
is complete.

Fixes: dcc9881e6767 ("RDMA/(core, ulp): Convert register/unregister event handler to be void")
Signed-off-by: William Kucharski <william.kucharski@oracle.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 58f70cfec45a..d35f021f154b 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -3204,7 +3204,6 @@ static int srpt_add_one(struct ib_device *device)
 
 	INIT_IB_EVENT_HANDLER(&sdev->event_handler, sdev->device,
 			      srpt_event_handler);
-	ib_register_event_handler(&sdev->event_handler);
 
 	for (i = 1; i <= sdev->device->phys_port_cnt; i++) {
 		sport = &sdev->port[i - 1];
@@ -3227,6 +3226,7 @@ static int srpt_add_one(struct ib_device *device)
 		}
 	}
 
+	ib_register_event_handler(&sdev->event_handler);
 	spin_lock(&srpt_dev_lock);
 	list_add_tail(&sdev->list, &srpt_dev_list);
 	spin_unlock(&srpt_dev_lock);
@@ -3237,7 +3237,6 @@ static int srpt_add_one(struct ib_device *device)
 
 err_port:
 	srpt_unregister_mad_agent(sdev, i);
-	ib_unregister_event_handler(&sdev->event_handler);
 err_cm:
 	if (sdev->cm_id)
 		ib_destroy_cm_id(sdev->cm_id);
-- 
2.39.3

Re: [PATCH v2 1/1] RDMA/srpt: Do not register event handler until srpt device is fully setup

[Leon Romanovsky]

On Fri, Feb 02, 2024 at 02:15:49AM -0700, William Kucharski wrote:
> Upon rare occasions, KASAN reports a use-after-free Write
> in srpt_refresh_port().
> 
> This seems to be because an event handler is registered before the
> srpt device is fully setup and a race condition upon error may leave a
> partially setup event handler in place.
> 
> Instead, only register the event handler after srpt device initialization
> is complete.
> 
> Fixes: dcc9881e6767 ("RDMA/(core, ulp): Convert register/unregister event handler to be void")

This is wrong line, the issues is much older.
Fixes: a42d985bd5b2 ("ib_srpt: Initial SRP Target merge for v3.3-rc1")

> Signed-off-by: William Kucharski <william.kucharski@oracle.com>

Please keep Reviewed-by tags provided by Bart.

Thanks

Re: [PATCH v2 0/1] RDMA/srpt: Do not register event handler until srpt device is fully setup

[Leon Romanovsky]

On Fri, 02 Feb 2024 02:15:48 -0700, William Kucharski wrote:
> Upon occasion, KASAN testing would report a use-after-free Write in
> srpt_refresh_port().
> 
> In the course of trying to diagnose this, I noticed that the code in
> srpt_add_one() registers an event handler for the srpt device and then
> initializes the ports on the device. If any portion of the
> device port initialization fails, it removes the registration for the
> event handler in the error leg.
> 
> [...]

Applied, thanks!

[1/1] RDMA/srpt: Do not register event handler until srpt device is fully setup
      https://git.kernel.org/rdma/rdma/c/c21a8870c98611

Best regards,
-- 
Leon Romanovsky <leon@kernel.org>