linux-kernel - Re: [PATCH 4.4 128/268] scsi: sd: Keep disk read-only when re-reading partition

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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 14 Jun 2018 18:50:07 +0000
From:   Jeremy Cline <jeremy@...ine.org>
To:     Ben Hutchings <ben.hutchings@...ethink.co.uk>,
        "Martin K. Petersen" <martin.petersen@...cle.com>
Cc:     stable@...r.kernel.org, Li Ning <lining916740672@...oud.com>,
        Sasha Levin <alexander.levin@...rosoft.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4.4 128/268] scsi: sd: Keep disk read-only when re-reading
 partition

Hi Ben,

On 06/13/2018 05:25 PM, Ben Hutchings wrote:
> On Mon, 2018-05-28 at 12:01 +0200, Greg Kroah-Hartman wrote:
>> 4.4-stable review patch.  If anyone has any objections, please let me know.
>>
>> ------------------
>>
>> From: Jeremy Cline <jeremy@...ine.org>
>>
>> [ Upstream commit 20bd1d026aacc5399464f8328f305985c493cde3 ]
>>
>> If the read-only flag is true on a SCSI disk, re-reading the partition
>> table sets the flag back to false.
>>
>> To observe this bug, you can run:
>>
>> 1. blockdev --setro /dev/sda
>> 2. blockdev --rereadpt /dev/sda
>> 3. blockdev --getro /dev/sda
>>
>> This commit reads the disk's old state and combines it with the device
>> disk-reported state rather than unconditionally marking it as RW.
> 
> It seems to me that this change is likely to cause a regression: if a
> SCSI device switches from read-only to read-write state then a
> subsequent rescan won't automatically change the block device to read-
> write state.  The administrator will have to use the blockdev command
> too.
> 
> Even if this change in behaviour is acceptable, this commit does not
> implement it consistently.  The function starts by clearing the ro flag
> and this commit only changes one of the three exit paths to preserve
> it.  (The log message about Write Protect status also reports the
> underlying SCSI device flag and not the combined ro flag, but maybe
> that was intentional.)

Yes, it looks like I messed this up.

> 
> I think this commit should be reverted, both in stable and upstream.  A
> proper fix would involve splitting the ro flag into two flags—one
> controlled by user-space and one read from the device—with the
> effective read-only status being the logical-or of those two.

This seems sensible to me, for whatever that's worth. I'm new to the
kernel so I'm not certain I'll produce a reasonable patch, but I'm
willing to give it a shot.

So the change would be something like:

1. Add a flag to the gendisk struct to indicate if the device is
   read-only. The driver sets this.

2. In hd_struct struct's policy flag is one that indicates the
   user-space setting and this gets set by BLKROSET ioctl/set_disk_ro.

3. The effective read-only status reported by the BLKROGET ioctl and
   bdev_read_only is the logical-or of the hd_struct policy flag and the
   gendisk device flag.

Maybe that doesn't make sense at all, though. Thoughts?


Thanks,
Jeremy