Re: Question regarding shared subtree mount flags.

[Ram Pai <linuxram@us.ibm.com>]

On Tue, Feb 28, 2012 at 10:33:15PM +0900, Tetsuo Handa wrote:
> I noticed that TOMOYO's mount flag checking priority
> 
>         if (flags & MS_REMOUNT) {
>                 type = tomoyo_mounts[TOMOYO_MOUNT_REMOUNT];
>                 flags &= ~MS_REMOUNT;
>         }
>         if (flags & MS_MOVE) {
>                 type = tomoyo_mounts[TOMOYO_MOUNT_MOVE];
>                 flags &= ~MS_MOVE;
>         }
>         if (flags & MS_BIND) {
>                 type = tomoyo_mounts[TOMOYO_MOUNT_BIND];
>                 flags &= ~MS_BIND;
>         }
>         if (flags & MS_UNBINDABLE) {
>                 type = tomoyo_mounts[TOMOYO_MOUNT_MAKE_UNBINDABLE];
>                 flags &= ~MS_UNBINDABLE;
>         }
>         if (flags & MS_PRIVATE) {
>                 type = tomoyo_mounts[TOMOYO_MOUNT_MAKE_PRIVATE];
>                 flags &= ~MS_PRIVATE;
>         }
>         if (flags & MS_SLAVE) {
>                 type = tomoyo_mounts[TOMOYO_MOUNT_MAKE_SLAVE];
>                 flags &= ~MS_SLAVE;
>         }
>         if (flags & MS_SHARED) {
>                 type = tomoyo_mounts[TOMOYO_MOUNT_MAKE_SHARED];
>                 flags &= ~MS_SHARED;
>         }
>         if (!type)
>                 type = "<NULL>";
>         idx = tomoyo_read_lock();
>         error = tomoyo_mount_acl(&r, dev_name, path, type, flags);
>         tomoyo_read_unlock(idx);
> 
> is wrong and it need to follow priority used by do_mount().
> 
>         if (flags & MS_REMOUNT)
>                 retval = do_remount(&path, flags & ~MS_REMOUNT, mnt_flags,
>                                     data_page);
>         else if (flags & MS_BIND)
>                 retval = do_loopback(&path, dev_name, flags & MS_REC);
>         else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
>                 retval = do_change_type(&path, flags);
>         else if (flags & MS_MOVE)
>                 retval = do_move_mount(&path, dev_name);
> 
> I got a question regarding priority among MS_SHARED, MS_PRIVATE, MS_SLAVE and
> MS_UNBINDABLE. I'm wondering which flag TOMOYO should use for permission check
> if more than one of MS_SHARED, MS_PRIVATE, MS_SLAVE or MS_UNBINDABLE is passed.
> 
>   commit 07b20889 "[PATCH] beginning of the shared-subtree proper"
>   commit 03e06e68 "[PATCH] introduce shared mounts"
>   commit a58b0eb8 "[PATCH] introduce slave mounts"
>   commit 9676f0c6 "[PATCH] unbindable mounts"
> 
> introduced these flags and
> 
>   commit 7a2e8a8f "VFS: Sanity check mount flags passed to change_mnt_propagation()"
> 
> clarified that these flags must be exclusively passed.

right. It makes more sense for a user to change a vfsmount to one type at a given
time.

> 
> Commit 7a2e8a8f went to 2.6.36 but has not gone to 2.6.27 nor 2.6.32.
> Is this simply because commit 7a2e8a8f was not proposed for backporting?

looks like it. No one saw the need to backport that patch.

> 
> If so, is security_sb_mount() allowed to return -EINVAL if more than one of
> these flags are passed (like shown below) so that TOMOYO will not generate
> inaccurate audit logs?
> 
>         if (flags & MS_REMOUNT) {
>                 type = tomoyo_mounts[TOMOYO_MOUNT_REMOUNT];
>                 flags &= ~MS_REMOUNT;
>         } else if (flags & MS_BIND) {
>                 type = tomoyo_mounts[TOMOYO_MOUNT_BIND];
>                 flags &= ~MS_BIND;
>         } else if (flags & MS_SHARED) {
>                 type = tomoyo_mounts[TOMOYO_MOUNT_MAKE_SHARED];
>                 flags &= ~MS_SHARED;
>                 if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
>                         return -EINVAL;
>         } else if (flags & MS_PRIVATE) {
>                 type = tomoyo_mounts[TOMOYO_MOUNT_MAKE_PRIVATE];
>                 flags &= ~MS_PRIVATE;
>                 if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
>                         return -EINVAL;
>         } else if (flags & MS_SLAVE) {
>                 type = tomoyo_mounts[TOMOYO_MOUNT_MAKE_SLAVE];
>                 flags &= ~MS_SLAVE;
>                 if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
>                         return -EINVAL;
>         } else if (flags & MS_UNBINDABLE) {
>                 type = tomoyo_mounts[TOMOYO_MOUNT_MAKE_UNBINDABLE];
>                 flags &= ~MS_UNBINDABLE;
>                 if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
>                         return -EINVAL;
>         } else if (flags & MS_MOVE) {
>                 type = tomoyo_mounts[TOMOYO_MOUNT_MOVE];
>                 flags &= ~MS_MOVE;
>         }

I  think that is right. However the code can be further reorganized to check for
(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE) only ones, instead of checking
under every else if.

RP

>         if (!type)
>                 type = "<NULL>";
>         idx = tomoyo_read_lock();
>         error = tomoyo_mount_acl(&r, dev_name, path, type, flags);
>         tomoyo_read_unlock(idx);