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);
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
