From patchwork Tue Apr 4 21:41:42 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jiri Kosina X-Patchwork-Id: 79395 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp177965vqo; Tue, 4 Apr 2023 14:54:36 -0700 (PDT) X-Google-Smtp-Source: AKy350bxPx3jUvHbxvuHl9RJ3RmU1WLgvy79qj7hEHGL1zIHJg8wxkMe3GbbN3k8rtqDb2QCNQBW X-Received: by 2002:a17:902:db07:b0:1a1:cc0c:a5c0 with SMTP id m7-20020a170902db0700b001a1cc0ca5c0mr4551240plx.63.1680645275966; Tue, 04 Apr 2023 14:54:35 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1680645275; cv=none; d=google.com; s=arc-20160816; b=XVCCHmnodI8J4mbpLgV3fgHlA44hq+BH4YPlO2BcSR+YeyP1S6DM8x5mVwuYAE7ENH BoH/G7ndXDlH2cZ+q3g7VveJwGqq4+HJ5Tq+NOefwtVQk7uvO+LMY4lplkr86dGPHrkh DDt53CornU9l/fUJsPhYopxgoN7dCbQUmg5nuGwDulrsNLxSE+fu7fMFidyx24nxgIKU +d5caII2sLH1WPANJUGjC36tdd2af5BMLK9UsHVs5wT5QhBV+o+othmHvysdCMBPxtE+ r4ummwEf/Pg+T2vH6TK2f6ndmKyVgoIoqlnMLbDsq6wNXQBWhefvXUlGq5oSfPdYkUd2 nJag== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :user-agent:references:message-id:in-reply-to:subject:cc:to:from :date:dkim-signature; bh=3V66YDHF3Gh40L1iVaTPBa7cIxZhQ7yMArQaH6LQBWY=; b=0iFffsVN5dL//RdnqH9PAfCqH88CN4XtoL8+HA12kx0uCZ2u/ZJdMEf1Xs7wYgMI5S WhoOKmkukNL8+x9nOxEfb5JbMiDd+mVz+4lCPsB/vf8ZCLnRV8n7KDUg1p/H9jvYoiOu zHDVpzvRVTEuFuD/VxhG1WA2NquCN5bGdWD719O1lmjtVI3OWZMK0iMIb0nwO10mi0g0 YCn7Gh8nj3/LpefA/F865y5W+imdYTLkXhyp77wSmzA3lVxloi5pFfjgjH2UciQkUR9A b9YjrKdMls77qa8Tg33KHao9YvuMQ0gase/mg49jMhl8SxOEx+vFFTASppJlvFoWKWop oL+Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=suGmChBU; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id b16-20020a170903229000b001a051676302si11516699plh.494.2023.04.04.14.54.24; Tue, 04 Apr 2023 14:54:35 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=suGmChBU; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236430AbjDDVlu (ORCPT + 99 others); Tue, 4 Apr 2023 17:41:50 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49156 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234988AbjDDVlt (ORCPT ); Tue, 4 Apr 2023 17:41:49 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C99753AB0; Tue, 4 Apr 2023 14:41:47 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 6456B639B3; Tue, 4 Apr 2023 21:41:47 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 12877C433D2; Tue, 4 Apr 2023 21:41:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1680644506; bh=lMszXUPs3xmT4/L30IdHdQrLlqYnTYyFa1K84xFbLzU=; h=Date:From:To:cc:Subject:In-Reply-To:References:From; b=suGmChBU3viKp/0FZp/PClBH3B/13p89RcgTbkDT3EsMOXC3oDkh6gYjPX2bapCZ/ aGs5TKU6V9bgcqh1+blinlxGO/YS49LgT+ZRsn9CSuZPUp3XhsRjKZLNJQERP6YH7n SH0zxfrw3GYMpps3+hAiPGqPJhzv05asqqtUeUY3Xpu/X0hkpApOyLdZR3MDWmeS9l bznh3G5HdNQLT0FjYySymJB7exOOe9Wsj+D0Gp1bOXwFubBCPhL4NtvoRFPcBS1Tka gIFnl1bh9qp5+7QWCvUEVBHU7dlLZ+y16JCBSFBb75449My+VcOMpvtX1EiA9MhlVW Y9JbxaO1APLFA== Date: Tue, 4 Apr 2023 23:41:42 +0200 (CEST) From: Jiri Kosina To: James Bottomley cc: "Martin K. Petersen" , linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, Ding Hui , Michal Kolar Subject: [PATCH v2] scsi: ses: Handle enclosure with just a primary component gracefully In-Reply-To: Message-ID: References: User-Agent: Alpine 2.21 (LSU 202 2017-01-01) MIME-Version: 1.0 X-Spam-Status: No, score=-2.5 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1762276424355911093?= X-GMAIL-MSGID: =?utf-8?q?1762284300560887285?= On Tue, 4 Apr 2023, James Bottomley 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 . > > 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. This hunk was taken from the original 2020 fix, but you are right, thanks for spotting this. Please find v2 below, with this hunk removed, and Tested-by: added. From: Jiri Kosina Subject: [PATCH] scsi: ses: Handle enclosure with just a primary component gracefully 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 . 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 Tested-by: Michal Kolar Originally-by: Ding Hui Cc: stable@vger.kernel.org Fixes: 3fe97ff3d9493 ("scsi: ses: Don't attach if enclosure has no components") Signed-off-by: Jiri Kosina --- v1 -> v2: - fix potential oops in ses_enclosure_find_by_addr() spotted by James - add Tested-by: drivers/scsi/ses.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index b11a9162e73a..f3fa92f493ec 100644 --- a/drivers/scsi/ses.c +++ b/drivers/scsi/ses.c @@ -602,8 +602,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 +736,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 +777,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);