Skip to contentSkip to conversation

scsi: scsi_dh_rdac: Avoid crash during rdac_bus_attach

PATCH Applied Ye Bin 3 participants 6 messages Jan 13, 2021 – Jul 29, 2021 SCSI

lore
Share & tools
lore

Ye Bin

author
#0 lore raw

PATCH v2 1/1 scsi: scsi_dh_rdac: Avoid crash during rdac_bus_attach

We get follow BUG_ON when rdac scan:

[595952.944297] kernel BUG at drivers/scsi/device_handler/scsi_dh_rdac.c:427!
[595952.951143] Internal error: Oops - BUG: 0 [#1] SMP
......
[595953.251065] Call trace:
[595953.259054]  check_ownership+0xb0/0x118
[595953.269794]  rdac_bus_attach+0x1f0/0x4b0
[595953.273787]  scsi_dh_handler_attach+0x3c/0xe8
[595953.278211]  scsi_dh_add_device+0xc4/0xe8
[595953.282291]  scsi_sysfs_add_sdev+0x8c/0x2a8
[595953.286544]  scsi_probe_and_add_lun+0x9fc/0xd00
[595953.291142]  __scsi_scan_target+0x598/0x630
[595953.295395]  scsi_scan_target+0x120/0x130
[595953.299481]  fc_user_scan+0x1a0/0x1c0 [scsi_transport_fc]
[595953.304944]  store_scan+0xb0/0x108
[595953.308420]  dev_attr_store+0x44/0x60
[595953.312160]  sysfs_kf_write+0x58/0x80
[595953.315893]  kernfs_fop_write+0xe8/0x1f0
[595953.319888]  __vfs_write+0x60/0x190
[595953.323448]  vfs_write+0xac/0x1c0
[595953.326836]  ksys_write+0x74/0xf0
[595953.330221]  __arm64_sys_write+0x24/0x30

BUG_ON code is in check_ownership:

                list_for_each_entry_rcu(tmp, &h->ctlr->dh_list, node) {
                        /* h->sdev should always be valid */
                        BUG_ON(!tmp->sdev);
                        tmp->sdev->access_state = access_state;
                }
rdac_bus_attach
	initialize_controller
		list_add_rcu(&h->node, &h->ctlr->dh_list);
		h->sdev = sdev;
rdac_bus_detach
	list_del_rcu(&h->node);
	h->sdev = NULL;

Test as follow steps:
(1) Find IO error, remove disk;
(2) Insert disk back;
(3) trigger scan disk;

There is race between rdac_bus_attach and rdac_bus_detach, maybe access rdac_dh_data which h->sdev has been set NULL when process rdac attach. And also find that "h->sdev" set value after add list, this may lead to reference NULL ptr.

Signed-off-by Ye Bin <[email protected]>
---
 drivers/scsi/device_handler/scsi_dh_rdac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/device_handler/scsi_dh_rdac.c +2 -2
index 5efc959493ec..85a71bafaea7 100644
@@ -453,8 +453,8 @@ static int initialize_controller(struct scsi_device *sdev,
 		if (!h->ctlr)
 			err = SCSI_DH_RES_TEMP_UNAVAIL;
 		else {
-			list_add_rcu(&h->node, &h->ctlr->dh_list);
 			h->sdev = sdev;
+			list_add_rcu(&h->node, &h->ctlr->dh_list);
 		}
 		spin_unlock(&list_lock);
 		err = SCSI_DH_OK;

@@ -778,11 +778,11 @@ static void rdac_bus_detach( struct scsi_device *sdev )
 	spin_lock(&list_lock);
 	if (h->ctlr) {
 		list_del_rcu(&h->node);
-		h->sdev = NULL;
 		kref_put(&h->ctlr->kref, release_controller);
 	}
 	spin_unlock(&list_lock);
 	sdev->handler_data = NULL;
+	synchronize_rcu();
 	kfree(h);
 }
Signature · 1 line
2.25.4
author
#1 lore raw
Quoted 68 lines · Ye Bin

We get follow BUG_ON when rdac scan:

[595952.944297] kernel BUG at drivers/scsi/device_handler/scsi_dh_rdac.c:427!
[595952.951143] Internal error: Oops - BUG: 0 [#1] SMP
......
[595953.251065] Call trace:
[595953.259054]  check_ownership+0xb0/0x118
[595953.269794]  rdac_bus_attach+0x1f0/0x4b0
[595953.273787]  scsi_dh_handler_attach+0x3c/0xe8
[595953.278211]  scsi_dh_add_device+0xc4/0xe8
[595953.282291]  scsi_sysfs_add_sdev+0x8c/0x2a8
[595953.286544]  scsi_probe_and_add_lun+0x9fc/0xd00
[595953.291142]  __scsi_scan_target+0x598/0x630
[595953.295395]  scsi_scan_target+0x120/0x130
[595953.299481]  fc_user_scan+0x1a0/0x1c0 [scsi_transport_fc]
[595953.304944]  store_scan+0xb0/0x108
[595953.308420]  dev_attr_store+0x44/0x60
[595953.312160]  sysfs_kf_write+0x58/0x80
[595953.315893]  kernfs_fop_write+0xe8/0x1f0
[595953.319888]  __vfs_write+0x60/0x190
[595953.323448]  vfs_write+0xac/0x1c0
[595953.326836]  ksys_write+0x74/0xf0
[595953.330221]  __arm64_sys_write+0x24/0x30

BUG_ON code is in check_ownership:

                 list_for_each_entry_rcu(tmp, &h->ctlr->dh_list, node) {
                         /* h->sdev should always be valid */
                         BUG_ON(!tmp->sdev);
                         tmp->sdev->access_state = access_state;
                 }
rdac_bus_attach
	initialize_controller
		list_add_rcu(&h->node, &h->ctlr->dh_list);
		h->sdev = sdev;
rdac_bus_detach
	list_del_rcu(&h->node);
	h->sdev = NULL;

Test as follow steps:
(1) Find IO error, remove disk;
(2) Insert disk back;
(3) trigger scan disk;

There is race between rdac_bus_attach and rdac_bus_detach, maybe access rdac_dh_data which h->sdev has been set NULL when process rdac attach. And also find that "h->sdev" set value after add list, this may lead to reference NULL ptr.

Signed-off-by Ye Bin <[email protected]>
---
  drivers/scsi/device_handler/scsi_dh_rdac.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/device_handler/scsi_dh_rdac.c +1 -1
index 5efc959493ec..85a71bafaea7 100644
@@ -453,8 +453,8 @@ static int initialize_controller(struct scsi_device *sdev,
  		if (!h->ctlr)
  			err = SCSI_DH_RES_TEMP_UNAVAIL;
  		else {
-			list_add_rcu(&h->node, &h->ctlr->dh_list);
  			h->sdev = sdev;
+			list_add_rcu(&h->node, &h->ctlr->dh_list);
  		}
  		spin_unlock(&list_lock);
  		err = SCSI_DH_OK;

@@ -778,11 +778,11 @@ static void rdac_bus_detach( struct scsi_device *sdev )
  	spin_lock(&list_lock);
  	if (h->ctlr) {

drivers/scsi/device_handler/scsi_dh_rdac.c static void rdac_bus_detach( struct scsi_device *sdev ) · line 788

Quoted 10 lines
drivers/scsi/device_handler/scsi_dh_rdac.c +1 -1
  		list_del_rcu(&h->node);
-		h->sdev = NULL;
  		kref_put(&h->ctlr->kref, release_controller);
  	}
  	spin_unlock(&list_lock);
  	sdev->handler_data = NULL;
+	synchronize_rcu();
  	kfree(h);
  }

ping ...

#2 lore raw
In reply to Ye Bin

drivers/scsi/device_handler/scsi_dh_rdac.c static void rdac_bus_detach( struct scsi_device *sdev ) · line 786

Quoted 3 lines · Ye Bin
drivers/scsi/device_handler/scsi_dh_rdac.c +1 -0
 	sdev->handler_data = NULL;
+	synchronize_rcu();
 	kfree(h);

What is the purpose of the new synchronize_rcu() call? If its purpose is to wait until *h is no longer in use, please use kfree_rcu() instead.

Thanks,

Bart.

author
#3 lore raw
Quoted 5 lines · Bart Van Assche

drivers/scsi/device_handler/scsi_dh_rdac.c static void rdac_bus_detach( struct scsi_device *sdev ) · line 786

Quoted 3 lines · Ye Bin
  	sdev->handler_data = NULL;
drivers/scsi/device_handler/scsi_dh_rdac.c +1 -0
+	synchronize_rcu();
  	kfree(h);

What is the purpose of the new synchronize_rcu() call?

Thanks for your reply.
Yes, I add new synchronize_rcu() call is to wait until *h is no longer in use. If free "h" right now , mybe lead to UAF.

Quoted 2 lines

If its purpose is
to wait until *h is no longer in use, please use kfree_rcu() instead.

struct rdac_dh_data {
        struct list_head        node;
        .....
}

As rdac_dh_data.node type is "struct list_head", but kfree_rcu the first parameter type is "struct rcu_head". So we can only use synchronize_rcu() at here.

Quoted 6 lines

Thanks,

Bart.
.

Reviewed-by
#4 lore raw
Quoted 9 lines · yebin
Quoted 5 lines · Bart Van Assche
Quoted 3 lines · Ye Bin

      sdev->handler_data = NULL;
+    synchronize_rcu();
      kfree(h);

What is the purpose of the new synchronize_rcu() call?

Thanks for your reply.
Yes, I add new synchronize_rcu() call is to wait until *h is no longer in use. If free "h" right now , mybe lead to UAF.

Quoted 8 lines
Quoted 2 lines

If its purpose is
to wait until *h is no longer in use, please use kfree_rcu() instead.

struct rdac_dh_data {
        struct list_head        node;
        .....
}
As rdac_dh_data.node type is "struct list_head", but  kfree_rcu the first parameter type is "struct rcu_head". So we can only use synchronize_rcu() at here.

Ah, that's right. Hence:

Reviewed-by Bart Van Assche <[email protected]>
Applied
#5 lore raw
In reply to Ye Bin
Quoted 23 lines · Ye Bin

We get follow BUG_ON when rdac scan:

[595952.944297] kernel BUG at drivers/scsi/device_handler/scsi_dh_rdac.c:427!
[595952.951143] Internal error: Oops - BUG: 0 [#1] SMP
......
[595953.251065] Call trace:
[595953.259054]  check_ownership+0xb0/0x118
[595953.269794]  rdac_bus_attach+0x1f0/0x4b0
[595953.273787]  scsi_dh_handler_attach+0x3c/0xe8
[595953.278211]  scsi_dh_add_device+0xc4/0xe8
[595953.282291]  scsi_sysfs_add_sdev+0x8c/0x2a8
[595953.286544]  scsi_probe_and_add_lun+0x9fc/0xd00
[595953.291142]  __scsi_scan_target+0x598/0x630
[595953.295395]  scsi_scan_target+0x120/0x130
[595953.299481]  fc_user_scan+0x1a0/0x1c0 [scsi_transport_fc]
[595953.304944]  store_scan+0xb0/0x108
[595953.308420]  dev_attr_store+0x44/0x60
[595953.312160]  sysfs_kf_write+0x58/0x80
[595953.315893]  kernfs_fop_write+0xe8/0x1f0
[595953.319888]  __vfs_write+0x60/0x190
[595953.323448]  vfs_write+0xac/0x1c0
[595953.326836]  ksys_write+0x74/0xf0
[595953.330221]  __arm64_sys_write+0x24/0x30
Quoted 1 line

[...]

Applied to 5.14/scsi-fixes, thanks!

[1/1] scsi: scsi_dh_rdac: Avoid crash during rdac_bus_attach

      https://git.kernel.org/mkp/scsi/c/fb5d909021b4
Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering