linux-kernel - Re: [PATCH 2/2] scsi: ufs: Protect PM ops and err

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1610111503.17820.1.camel@mtkswgap22>
Date:   Fri, 8 Jan 2021 21:11:43 +0800
From:   Stanley Chu <stanley.chu@...iatek.com>
To:     Bean Huo <huobean@...il.com>
CC:     Can Guo <cang@...eaurora.org>, <asutoshd@...eaurora.org>,
        <nguyenb@...eaurora.org>, <hongwus@...eaurora.org>,
        <ziqichen@...eaurora.org>, <rnayak@...eaurora.org>,
        <linux-scsi@...r.kernel.org>, <kernel-team@...roid.com>,
        <saravanak@...gle.com>, <salyzyn@...gle.com>, <rjw@...ysocki.net>,
        Alim Akhtar <alim.akhtar@...sung.com>,
        Avri Altman <avri.altman@....com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Bean Huo <beanhuo@...ron.com>,
        Nitin Rawat <nitirawa@...eaurora.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        "Bart Van Assche" <bvanassche@....org>,
        Satya Tangirala <satyat@...gle.com>,
        "open list" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] scsi: ufs: Protect PM ops and err_handler from user
 access through sysfs

Hi Bean,

On Fri, 2021-01-08 at 12:29 +0100, Bean Huo wrote:
> On Wed, 2021-01-06 at 09:20 +0800, Can Guo wrote:
> > Hi Bean,
> > 
> > On 2021-01-06 02:38, Bean Huo wrote:
> > > On Tue, 2021-01-05 at 09:07 +0800, Can Guo wrote:
> > > > On 2021-01-05 04:05, Bean Huo wrote:
> > > > > On Sat, 2021-01-02 at 05:59 -0800, Can Guo wrote:
> > > > > > + * @shutting_down: flag to check if shutdown has been
> > > > > > invoked
> > > > > 
> > > > > I am not much sure if this flag is need, since once PM going in
> > > > > shutdown path, what will be returnded by pm_runtime_get_sync()?
> > > > > 
> > > > > If pm_runtime_get_sync() will fail, just check its return.
> > > > > 
> > > > 
> > > > That depends. During/after shutdown, for UFS's case only,
> > > > pm_runtime_get_sync(hba->dev) will most likely return 0,
> > > > because it is already RUNTIME_ACTIVE, pm_runtime_get_sync()
> > > > will directly return 0... meaning you cannot count on it.
> > > > 
> > > > Check Stanley's change -
> > > > https://lore.kernel.org/patchwork/patch/1341389/
> > > > 
> > > > Can Guo.
> > > 
> > > Can,
> > > 
> > > Thanks for pointing out that.
> > > 
> > > Based on my understanding, that patch is redundent. maybe I
> > > misundestood Linux shutdown sequence.
> > 
> > Sorry, do you mean Stanley's change is redundant?
> 
> yes.
> 
> > 
> > > 
> > > I checked the shutdown flow:
> > > 
> > > 1. Set the "system_state" variable
> > > 2. Disable usermod to ensure that no user from userspace can start
> > > a
> > > request
> > 
> > I hope it is like what you interpreted, but step #2 only stops
> > UMH(#265)
> > but not all user space activities. Whereas, UMH is for kernel space 
> > calling
> > user space.
> 
> 
> Can,
> 
> I did further study and homework on the Linux shutdown in the last few
> days. Yes, you are right, usermodehelper_disable() is to prevent
> executing the process from the kernel space.
> 
> But I didn't reproduce this "maybe" race issue while shutdown. no
> matter how I torment my system, once Linux shutdown/halt/reboot starts,
> nobody can access the sysfs node. I create 10 processes in the user
> space and constantly access UFS sysfs node, also, fio is running in the
> background for the normal data read/write. there is a shutdown thread
> that will randomly trigger shutdown/halt/reboot. but no race issue
> appears.
> 
> I don't know if this is a hypothetical issue(the race between shutdown
> flow and sysfs node access), it may not really exist in the Linux
> envriroment. everytime, the shutdonw flow will be:
> 
> e10_sync_handler()->e10_svc()->do_e10_svc()->__do_sys_reboot()-
> >kernel_poweroff/kernel_halt()->device_shutdown()->platform_shutdown()-
> >ufshcd_platform_shutdown()->ufshcd_shutdown().
> 
> I think before going into the kernel shutdown, the userspace cannot
> issue new requests anymore. otherwise, this would be a big issue.
> 
> pm_runtime_get_sync() will return 0 or failure while shutdown? the
> answer is not important now, maybe as you said, it is always 0. But in
> my testing, it didn't get there the system has been shutdown. Which
> means once shutdonw starts, sysfs node access path cannot reach
> pm_runtime_get_sync(). (note, I don't know if sysfs node access thread
> has been disabled or not)
> 
> 
> Responsibly say, I didn't reproduce this issue on my system (ubuntu),
> maybe you are using Android. I am not an expert on this topic, if you
> have the best idea on how to reproduce this issue. please please let me
> try. appreciate it!!!!!
> 

One of the racing in my platforms happens due to I/O access triggered
from kernel space, not user space.

Thanks,
Stanley Chu

> 
> Thanks,
> Bean
> 
> 
> > 
> > 264     system_state = state;
> > 265     usermodehelper_disable();
> > 266     device_shutdown();
> > 
> > Thanks,
> > Can Guo.
>