Re: [PATCH] block: Add config option to not allow writing to mounted devices

[Jan Kara <jack@suse.cz>]

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@linaro.org
> >>> Signed-off-by: Jan Kara <jack@suse.cz>
> >> 
> >> 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.

> 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@suse.com>
SUSE Labs, CR