Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <c5deac044ac409e32d9ad9968ce0dcbc996bfc7a.camel@linux.ibm.com>
Subject: Re: [PATCH] scsi: ses: Fix crash caused by kfree an invalid pointer
From:   James Bottomley <jejb@linux.ibm.com>
Reply-To: jejb@linux.ibm.com
To:     Ding Hui <dinghui@sangfor.com.cn>, martin.petersen@oracle.com
Cc:     linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        stable <stable@vger.kernel.org>
Date:   Sat, 28 Nov 2020 15:27:21 -0800
In-Reply-To: <20201128122302.9490-1-dinghui@sangfor.com.cn>
References: <20201128122302.9490-1-dinghui@sangfor.com.cn>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.34.4 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sat, 2020-11-28 at 20:23 +0800, Ding Hui wrote:
> 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
> 
> Tested-by: Zeng Zhicong <timmyzeng@163.com>
> Cc: stable <stable@vger.kernel.org> # 2.6.25+
> Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>

This doesn't really look to be the right thing to do: an enclosure
which has no component can't usefully be controlled by the driver since
there's nothing for it to do, so what we should do in this situation is
refuse to attach like the proposed patch below.

It does seem a bit odd that someone would build an enclosure that
doesn't enclose anything, so would you mind running

sg_ses -e 

on it and reporting back what it shows?  It's possible there's another
type that the enclosure device should be tracking.

Regards,

James

---8>8>8><8<8<8--------
From: James Bottomley <James.Bottomley@HansenPartnership.com>
Subject: [PATCH] scsi: ses: don't attach if enclosure has no components

An enclosure with no components can't usefully be operated by the
driver (since effectively it has nothing to manage), so report the
problem and don't attach.  Not attaching also fixes an oops which
could occur if the driver tries to manage a zero component enclosure.

Reported-by: Ding Hui <dinghui@sangfor.com.cn>
Cc: stable@vger.kernel.org
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/ses.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index c2afba2a5414..9624298b9c89 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -690,6 +690,11 @@ static int ses_intf_add(struct device *cdev,
 		    type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE)
 			components += type_ptr[1];
 	}
+	if (components == 0) {
+		sdev_printk(KERN_ERR, sdev, "enclosure has no enumerated components\n");
+		goto err_free;
+	}
+
 	ses_dev->page1 = buf;
 	ses_dev->page1_len = len;
 	buf = NULL;
-- 
2.26.2