On Tue, Jun 13, 2023 at 01:34:48PM +0200, Jan Kara wrote: > On Mon 12-06-23 14:52:54, Colin Walters wrote: > > On Mon, Jun 12, 2023, at 1:39 PM, Bart Van Assche wrote: > > > On 6/12/23 09:25, Jan Kara wrote: > > >> On Mon 12-06-23 18:16:14, Jan Kara wrote: > > >>> Writing to mounted devices is dangerous and can lead to filesystem > > >>> corruption as well as crashes. Furthermore syzbot comes with more and > > >>> more involved examples how to corrupt block device under a mounted > > >>> filesystem leading to kernel crashes and reports we can do nothing > > >>> about. Add config option to disallow writing to mounted (exclusively > > >>> open) block devices. Syzbot can use this option to avoid uninteresting > > >>> crashes. Also users whose userspace setup does not need writing to > > >>> mounted block devices can set this config option for hardening. > > >>> > > >>> Link: https://lore.kernel.org/all/60788e5d-5c7c-1142-e554-c21d709acfd9@xxxxxxxxxx > > >>> Signed-off-by: Jan Kara <jack@xxxxxxx> > > >> > > >> Please disregard this patch. I had uncommited fixups in my tree. I'll send > > >> fixed version shortly. I'm sorry for the noise. > > > > > > Have alternatives been configured to making this functionality configurable > > > at build time only? How about a kernel command line parameter instead of a > > > config option? > > > > It's not just syzbot here; at least once in my life I accidentally did > > `dd if=/path/to/foo.iso of=/dev/sda` when `/dev/sda` was my booted disk > > and not the target USB device. I know I'm not alone =) > > Yeah, so I'm not sure we are going to protect against this particular case. > I mean it is not *that* uncommon to alter partition table of /dev/sda while > /dev/sda1 is mounted. And for the kernel it is difficult to distinguish > this and your mishap. Honestly? I'd love it if filesystems actually /could/ lock down the parts of block devices they're using. They could hand out write privileges to the open bdev fds at the same time that a block layout lease is created, and retract them when the lease terminates. Areas before the fs (e.g. BIOS boot sector) could actually be left writable by filesystems that don't use that area; and anything beyond EOFS would still be writable (hello lvm). Then xfs actually /could/ prevent you from blowing away mounted xfs filesystem. ext4 could even still allow primary superblock writes to avoid breaking tune2fs, or they could detect secureboot lockdown and prohibit that. In the past, I was told to go write an LSM if I wanted XFS to protect itself from getting nuked, but I've been too busy to learn how to do that. The other nastier question is blocking writes to sda when sda1 is mounted; for that I have no response. :( --D > > There's a lot of similar accidental-damage protection from this. Another > > stronger argument here is that if one has a security policy that > > restricts access to filesystem level objects, if a process can somehow > > write to a mounted block device, it effectively subverts all of those > > controls. > > Well, there are multiple levels of protection that I can think of: > > 1) If user can write some image and make kernel mount it. > 2) If user can modify device content while mounted (but not buffer cache > of the device). > 3) If user can modify buffer cache of the device while mounted. > > 3) is the most problematic and effectively equivalent to full machine > control (executing arbitrary code in kernel mode) these days. For 1) and > 2) there are reasonable protection measures the filesystem driver can take > (and effectively you cannot escape these problems if you allow attaching > untrusted devices such as USB sticks) so they can cause DoS but we should > be able to prevent full machine takeover in the filesystem code. > > So this patch is mainly aimed at forbiding 3). > > > Right now it looks to me we're invoking devcgroup_check_permission pretty > > early on; maybe we could extend the device cgroup stuff to have a new > > check for write-mounted, like > > > > ``` > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > > index c994ff5b157c..f2af33c5acc1 100644 > > --- a/tools/include/uapi/linux/bpf.h > > +++ b/tools/include/uapi/linux/bpf.h > > @@ -6797,6 +6797,7 @@ enum { > > BPF_DEVCG_ACC_MKNOD = (1ULL << 0), > > BPF_DEVCG_ACC_READ = (1ULL << 1), > > BPF_DEVCG_ACC_WRITE = (1ULL << 2), > > + BPF_DEVCG_ACC_WRITE_MOUNTED = (1ULL << 3), > > }; > > > > enum { > > ``` > > > > ? But probably this would need to be some kind of opt-in flag to avoid > > breaking existing bpf progs? > > > > If it was configurable via the device cgroup, then it's completely > > flexible from userspace; most specifically including supporting some > > specially privileged processes from doing it if necessary. > > I kind of like the flexibility of device cgroups but it does not seem to > fit well with my "stop unactionable syzbot reports" usecase and doing the > protection properly would mean that we now need to create way to approve > access for all the tools that need this. So I'm not against this but I'd > consider this "future extension possibility" :). > > > Also, I wonder if we should also support restricting *reads* from mounted > > block devices? > > I don't see a strong usecase for this. Why would mounted vs unmounted > matter here? > > Honza > -- > Jan Kara <jack@xxxxxxxx> > SUSE Labs, CR
