diff mbox series

scsi: ses: Handle enclosure with just a primary component gracefully

Message ID nycvar.YFH.7.76.2304042122270.29760@cbobk.fhfr.pm
State New
Headers
Series scsi: ses: Handle enclosure with just a primary component gracefully |

Commit Message

Jiri Kosina April 4, 2023, 7:23 p.m. UTC 
  From: Jiri Kosina <jkosina@suse.cz>

This reverts 3fe97ff3d9493 ("scsi: ses: Don't attach if enclosure has no
components") and introduces proper handling of case where there are no detected
secondary components, but primary component (enumerated in num_enclosures)
does exist. That fix was originally proposed by Ding Hui <dinghui@sangfor.com.cn>.

Completely ignoring devices that have one primary enclosure and no secondary one
results in ses_intf_add() bailing completely

	scsi 2:0:0:254: enclosure has no enumerated components
        scsi 2:0:0:254: Failed to bind enclosure -12ven in valid configurations such

even on valid configurations with 1 primary and 0 secondary enclosures as below:

	# sg_ses /dev/sg0
	  3PARdata  SES               3321
	Supported diagnostic pages:
	  Supported Diagnostic Pages [sdp] [0x0]
	  Configuration (SES) [cf] [0x1]
	  Short Enclosure Status (SES) [ses] [0x8]
	# sg_ses -p cf /dev/sg0
	  3PARdata  SES               3321
	Configuration diagnostic page:
	  number of secondary subenclosures: 0
	  generation code: 0x0
	  enclosure descriptor list
	    Subenclosure identifier: 0 [primary]
	      relative ES process id: 0, number of ES processes: 1
	      number of type descriptor headers: 1
	      enclosure logical identifier (hex): 20000002ac02068d
	      enclosure vendor: 3PARdata  product: VV                rev: 3321
	  type descriptor header and text list
	    Element type: Unspecified, subenclosure id: 0
	      number of possible elements: 1

The changelog for the original fix follows

=====
We can get a crash when disconnecting the iSCSI session,
the call trace like this:

  [ffff00002a00fb70] kfree at ffff00000830e224
  [ffff00002a00fba0] ses_intf_remove at ffff000001f200e4
  [ffff00002a00fbd0] device_del at ffff0000086b6a98
  [ffff00002a00fc50] device_unregister at ffff0000086b6d58
  [ffff00002a00fc70] __scsi_remove_device at ffff00000870608c
  [ffff00002a00fca0] scsi_remove_device at ffff000008706134
  [ffff00002a00fcc0] __scsi_remove_target at ffff0000087062e4
  [ffff00002a00fd10] scsi_remove_target at ffff0000087064c0
  [ffff00002a00fd70] __iscsi_unbind_session at ffff000001c872c4
  [ffff00002a00fdb0] process_one_work at ffff00000810f35c
  [ffff00002a00fe00] worker_thread at ffff00000810f648
  [ffff00002a00fe70] kthread at ffff000008116e98

In ses_intf_add, components count could be 0, and kcalloc 0 size scomp,
but not saved in edev->component[i].scratch

In this situation, edev->component[0].scratch is an invalid pointer,
when kfree it in ses_intf_remove_enclosure, a crash like above would happen
The call trace also could be other random cases when kfree cannot catch
the invalid pointer

We should not use edev->component[] array when the components count is 0
We also need check index when use edev->component[] array in
ses_enclosure_data_process
=====

Reported-by: Michal Kolar <mich.k@seznam.cz>
Originally-by: Ding Hui <dinghui@sangfor.com.cn>
Cc: stable@vger.kernel.org
Fixes: 3fe97ff3d9493 ("scsi: ses: Don't attach if enclosure has no components")
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 drivers/scsi/ses.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

Comments

James Bottomley April 4, 2023, 8:26 p.m. UTC | #1 
On Tue, 2023-04-04 at 21:23 +0200, Jiri Kosina wrote:
> From: Jiri Kosina <jkosina@suse.cz>
> 
> This reverts 3fe97ff3d9493 ("scsi: ses: Don't attach if enclosure has
> no components") and introduces proper handling of case where there
> are no detected secondary components, but primary component
> (enumerated in num_enclosures) does exist. That fix was originally
> proposed by Ding Hui <dinghui@sangfor.com.cn>.

I think everything in here looks fine except this:

> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -509,9 +509,6 @@ static int ses_enclosure_find_by_addr(struct
> enclosure_device *edev,
>         int i;
>         struct ses_component *scomp;
>  
> -       if (!edev->component[0].scratch)
> -               return 0;
> -
>         for (i = 0; i < edev->components; i++) {
>                 scomp = edev->component[i].scratch;
>                 if (scomp->addr != efd->addr)

If you remove the check, then scomp could be NULL here and we'll oops
on scomp->addr.

Regards,

James
Ding Hui April 5, 2023, 4:27 a.m. UTC | #2 
On 2023/4/5 4:26, James Bottomley wrote:
> On Tue, 2023-04-04 at 21:23 +0200, Jiri Kosina wrote:
>> From: Jiri Kosina <jkosina@suse.cz>
>>
>> This reverts 3fe97ff3d9493 ("scsi: ses: Don't attach if enclosure has
>> no components") and introduces proper handling of case where there
>> are no detected secondary components, but primary component
>> (enumerated in num_enclosures) does exist. That fix was originally
>> proposed by Ding Hui <dinghui@sangfor.com.cn>.
> 
> I think everything in here looks fine except this:
> 
>> --- a/drivers/scsi/ses.c
>> +++ b/drivers/scsi/ses.c
>> @@ -509,9 +509,6 @@ static int ses_enclosure_find_by_addr(struct
>> enclosure_device *edev,
>>          int i;
>>          struct ses_component *scomp;
>>   
>> -       if (!edev->component[0].scratch)
>> -               return 0;
>> -
>>          for (i = 0; i < edev->components; i++) {
>>                  scomp = edev->component[i].scratch;
>>                  if (scomp->addr != efd->addr)
> 
> If you remove the check, then scomp could be NULL here and we'll oops
> on scomp->addr.

I think we should remove the check, because the edev->components 
represented the effectiveness of array pointers, so we need check 
edev->components firstly instead of checking edev->component[0].scratch, 
if edev->components is 0, we won't enter the for loop, don't worry about 
dereference scomp.

> 
> Regards,
> 
> James
> 
>
Jiri Kosina April 9, 2023, 10:38 p.m. UTC | #3 
On Wed, 5 Apr 2023, Ding Hui wrote:

> >> This reverts 3fe97ff3d9493 ("scsi: ses: Don't attach if enclosure has
> >> no components") and introduces proper handling of case where there
> >> are no detected secondary components, but primary component
> >> (enumerated in num_enclosures) does exist. That fix was originally
> >> proposed by Ding Hui <dinghui@sangfor.com.cn>.
> > 
> > I think everything in here looks fine except this:
> > 
> >> --- a/drivers/scsi/ses.c
> >> +++ b/drivers/scsi/ses.c
> >> @@ -509,9 +509,6 @@ static int ses_enclosure_find_by_addr(struct
> >> enclosure_device *edev,
> >>          int i;
> >>          struct ses_component *scomp;
> >>   -       if (!edev->component[0].scratch)
> >> -               return 0;
> >> -
> >>          for (i = 0; i < edev->components; i++) {
> >>                  scomp = edev->component[i].scratch;
> >>                  if (scomp->addr != efd->addr)
> > 
> > If you remove the check, then scomp could be NULL here and we'll oops
> > on scomp->addr.
> 
> I think we should remove the check, because the edev->components 
> represented the effectiveness of array pointers, so we need check 
> edev->components firstly instead of checking edev->component[0].scratch, 
> if edev->components is 0, we won't enter the for loop, don't worry about 
> dereference scomp.

Right you are. So v1 is actually more correct.

Martin, could you please consider adding

	Tested-by: Michal Kolar <mich.k@seznam.cz>

to v1 and applying? Thanks,
Martin K. Petersen April 12, 2023, 2:03 a.m. UTC | #4 
On Tue, 04 Apr 2023 21:23:42 +0200, Jiri Kosina wrote:

> This reverts 3fe97ff3d9493 ("scsi: ses: Don't attach if enclosure has no
> components") and introduces proper handling of case where there are no detected
> secondary components, but primary component (enumerated in num_enclosures)
> does exist. That fix was originally proposed by Ding Hui <dinghui@sangfor.com.cn>.
> 
> Completely ignoring devices that have one primary enclosure and no secondary one
> results in ses_intf_add() bailing completely
> 
> [...]

Applied to 6.3/scsi-fixes, thanks!

[1/1] scsi: ses: Handle enclosure with just a primary component gracefully
      https://git.kernel.org/mkp/scsi/c/c8e22b7a1694
diff mbox series

Patch

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index b11a9162e73a..b54f2c6c08c3 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -509,9 +509,6 @@  static int ses_enclosure_find_by_addr(struct enclosure_device *edev,
 	int i;
 	struct ses_component *scomp;
 
-	if (!edev->component[0].scratch)
-		return 0;
-
 	for (i = 0; i < edev->components; i++) {
 		scomp = edev->component[i].scratch;
 		if (scomp->addr != efd->addr)
@@ -602,8 +599,10 @@  static void ses_enclosure_data_process(struct enclosure_device *edev,
 						components++,
 						type_ptr[0],
 						name);
-				else
+				else if (components < edev->components)
 					ecomp = &edev->component[components++];
+				else
+					ecomp = ERR_PTR(-EINVAL);
 
 				if (!IS_ERR(ecomp)) {
 					if (addl_desc_ptr) {
@@ -734,11 +733,6 @@  static int ses_intf_add(struct device *cdev,
 			components += type_ptr[1];
 	}
 
-	if (components == 0) {
-		sdev_printk(KERN_WARNING, sdev, "enclosure has no enumerated components\n");
-		goto err_free;
-	}
-
 	ses_dev->page1 = buf;
 	ses_dev->page1_len = len;
 	buf = NULL;
@@ -780,9 +774,11 @@  static int ses_intf_add(struct device *cdev,
 		buf = NULL;
 	}
 page2_not_supported:
-	scomp = kcalloc(components, sizeof(struct ses_component), GFP_KERNEL);
-	if (!scomp)
-		goto err_free;
+	if (components > 0) {
+		scomp = kcalloc(components, sizeof(struct ses_component), GFP_KERNEL);
+		if (!scomp)
+			goto err_free;
+	}
 
 	edev = enclosure_register(cdev->parent, dev_name(&sdev->sdev_gendev),
 				  components, &ses_enclosure_callbacks);