From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933716AbeEIF3g (ORCPT ); Wed, 9 May 2018 01:29:36 -0400 Received: from mta-p4.oit.umn.edu ([134.84.196.204]:52776 "EHLO mta-p4.oit.umn.edu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933457AbeEIF3f (ORCPT ); Wed, 9 May 2018 01:29:35 -0400 X-Google-Smtp-Source: AB8JxZqnk+hu39WJzwODSmv53uBuaCSZ4sKGFMBpgX3l0U2IgTQQEOe/NW8MamYbPLUMKdysFnioaLOgOXJarUFY5UE= MIME-Version: 1.0 In-Reply-To: <1525501896-8235-1-git-send-email-wang6495@umn.edu> References: <1525501896-8235-1-git-send-email-wang6495@umn.edu> From: Wenwen Wang Date: Wed, 9 May 2018 00:28:53 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] scsi: mpt3sas: fix a missing-check bug To: Wenwen Wang Cc: Kangjie Lu , Sathya Prakash , Chaitra P B , Suganath Prabu Subramani , "James E.J. Bottomley" , "Martin K. Petersen" , "open list:LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)" , "open list:LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)" , open list Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Could you please review this patch? We need a confirmation because we are working on an approaching deadline. Thanks! Wenwen On Sat, May 5, 2018 at 1:31 AM, Wenwen Wang wrote: > In _ctl_ioctl_main(), 'ioctl_header' is first copied from the userspace > pointer 'arg'. 'ioctl_header.ioc_number' is then verified by > _ctl_verify_adapter(). If the verification is failed, an error code -ENODEV > is returned. Otherwise, the verification result, i.e., the MPT3SAS adapter > that matches with the 'ioctl_header.ioc_number', is saved to 'ioc'. Later > on, if the 'cmd' is MPT3COMMAND, the whole ioctl command struct is copied > again from the userspace pointer 'arg' and saved to 'karg'. Then the > function _ctl_do_mpt_command() is invoked to execute the command with the > adapter 'ioc' and 'karg' as inputs. > > Given that the pointer 'arg' resides in userspace, a malicious userspace > process can race to change the 'ioc_number' between the two copies, which > will cause inconsistency issues, potentially security issues since an > inconsistent adapter could be used with the command struct 'karg' as inputs > of _ctl_do_mpt_command(). Moreover, the user can potentially provide a > valid 'ioc_number' to pass the verification, and then modify it to an > invalid 'ioc_number'. That means, an invalid 'ioc_number' can potentially > bypass the verification check. > > To fix this issue, we need to recheck the 'ioc_number' copied after the > second copy to make sure it is not changed since the first copy. > > Signed-off-by: Wenwen Wang > --- > drivers/scsi/mpt3sas/mpt3sas_ctl.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > index d3cb387..0c140c7 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > @@ -2388,6 +2388,11 @@ _ctl_ioctl_main(struct file *file, unsigned int cmd, void __user *arg, > break; > } > > + if (karg.hdr.ioc_number != ioctl_header.ioc_number) { > + ret = -EINVAL; > + break; > + } > + > if (_IOC_SIZE(cmd) == sizeof(struct mpt3_ioctl_command)) { > uarg = arg; > ret = _ctl_do_mpt_command(ioc, karg, &uarg->mf); > -- > 2.7.4 >