linux-kernel - Re: Read-only `slaves` with shared subtrees?

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: <20170920193954.GK5698@ram.oc3035372033.ibm.com>
Date:   Wed, 20 Sep 2017 12:39:54 -0700
From:   Ram Pai <linuxram@...ibm.com>
To:     Dawid Ciezarkiewicz <dawid.ciezarkiewicz@...rik.com>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: Read-only `slaves` with shared subtrees?

On Tue, Sep 19, 2017 at 04:18:02PM -0700, Dawid Ciezarkiewicz wrote:
> On Mon, Sep 18, 2017 at 1:47 PM, Ram Pai <linuxram@...ibm.com> wrote:
> > It is possible to make a slave mount readonly, by  remounting it with
> > 'ro' flags.
> >
> > something like
> >
> > mount -o bind,remount,ro <slave-mount-dir>
> >
> > Any mount-propagation events reaching a read-only-slave does
> > inherit the slave attribute. However it does not inherit the
> > read-only attribute.
> 
> I did try manually remounting, and it worked for me. If this could be
> done atomically
>  (which I assume can't be, in the userspace) it could even be a workaround.
> 
> > Should it inherit? or should it not? -- that has not been thought
> > off AFAICT. it think we should let it inherit.
> 
> It makes sense, and it would work in my use-case. I wonder
> if that would break any existing expectations though.

It could break existing expectations, for mounts created by propagation.
This needs to be thought through. Also Should the same semantics
apply to MNT_NOSUID, MNT_NOEXEC etc etc? 

Copying Eric. he should be able to tell if any of the container
infrastructure assumes anything about mounts propagated to read-only
mounts.


> 
> I could at least test such a patch, it seems like a tiny change.
> Should I give it a try and submit a patch? If you could PM me any pointers
> it could help a lot since I'm not familiar with FS internals. So far I got here:

Here is a rough patch which will accomplish what you want; not
compile-tested nor tested.


diff --git a/fs/namespace.c b/fs/namespace.c
index f8893dc..3239adc 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1061,6 +1061,9 @@ static struct mount *clone_mnt(struct mount *old, struct dentry *root,
 	list_add_tail(&mnt->mnt_instance, &sb->s_mounts);
 	unlock_mount_hash();
 
+	if (flag & CL_READONLY)
+		mnt->mnt.mnt_flags |= MNT_READONLY;
+
 	if ((flag & CL_SLAVE) ||
 	    ((flag & CL_SHARED_TO_SLAVE) && IS_MNT_SHARED(old))) {
 		list_add(&mnt->mnt_slave, &old->mnt_slave_list);
diff --git a/fs/pnode.c b/fs/pnode.c
index 53d411a..aeb5b47 100644
--- a/fs/pnode.c
+++ b/fs/pnode.c
@@ -262,6 +262,8 @@ static int propagate_one(struct mount *m)
 	/* Notice when we are propagating across user namespaces */
 	if (m->mnt_ns->user_ns != user_ns)
 		type |= CL_UNPRIVILEGED;
+	if (m->mnt.mnt_flags & MNT_READONLY)
+		type |= CL_READONLY;
 	child = copy_tree(last_source, last_source->mnt.mnt_root, type);
 	if (IS_ERR(child))
 		return PTR_ERR(child);
diff --git a/fs/pnode.h b/fs/pnode.h
index dc87e65..7c59469 100644
--- a/fs/pnode.h
+++ b/fs/pnode.h
@@ -29,6 +29,7 @@
 #define CL_SHARED_TO_SLAVE	0x20
 #define CL_UNPRIVILEGED		0x40
 #define CL_COPY_MNT_NS_FILE	0x80
+#define CL_READONLY		0x100
 
 #define CL_COPY_ALL		(CL_COPY_UNBINDABLE | CL_COPY_MNT_NS_FILE)
 
RP