cd9660: Preserve non-access permission bits in file modes

[jhb]

Summary

Specifically, don't inadvertently mask off setuid, setgid, and sticky
bits matching the description of the dirmask option in the manual.

Reported by: stevek
Fixes: 82f2275b73e5 cd9660: Add support for mask,dirmask,uid,gid options

[stevek]

sys/fs/cd9660/cd9660_vnops.c

148

This needs parenthesis around the RHS of the or to quiet warnings.

186

Same here

[jhb]

Adjust ()s to quiet warnings

[olce]

It seems that the commit "fixed" by this revision introduced into mount_cd9660(8) -m and -M options exactly modeled after those of mount_msdosfs(8), as I gather from the exact words in the manual pages, in particular the sentence:

Only the nine low-order bits of mask are used.

However, I think this description for msdosfs(4) is misleading. I suspect it is here because, in msdosfs_access(), all 'rwx' flags are set by default and can then be removed by the mask. In particular, the sticky, setuid and setgid bits can never be set.

For cd9660(4), the situation seems similar, as I couldn't find any code that set flags outside of ACCESSPERMS. But if you think there is some, then it would be probably better to just apply the fully passed mask.

To wrap up, I'd suggest giving up on this revision and instead simply delete the quoted sentence above both from mount_cd9660(8) and mount_msdosfs(8).

[stevek]

I was okay with not having the mask have an effect on the suid/sgid because there's already the mount option for nosuid (which disables suid and sgid both), so there's not really a need for the mask to alter it.

[jhb]

@olce Rock Ridge extensions support extended modes like sticky bits on ISO images. msdosfs has no way to store such bits and the permissions are all synthetic for msdosfs.

e.g.:

/*
 * POSIX file attribute
 */
static int
cd9660_rrip_attr(ISO_RRIP_ATTR *p, ISO_RRIP_ANALYZE *ana)
{
	ana->inop->inode.iso_mode = isonum_733(p->mode);
	ana->inop->inode.iso_uid = isonum_733(p->uid);
	ana->inop->inode.iso_gid = isonum_733(p->gid);
	ana->inop->inode.iso_links = isonum_733(p->links);
	ana->fields &= ~ISO_SUSP_ATTR;
	return ISO_SUSP_ATTR;
}

This supports the full 16 bits of mode_t (and it's actually stored as a 32 byte integer).

[olce]

In D46948#1074919, @jhb wrote:

@olce Rock Ridge extensions support extended modes like sticky bits on ISO images.

Ah thanks, missed that.

Shouldn't the masks allow to clear the non-ACCESSPERMS bits? Or are you considering that redundant with, e.g., the nosuid mount option?

[jhb]

In D46948#1074925, @olce wrote:

In D46948#1074919, @jhb wrote:

@olce Rock Ridge extensions support extended modes like sticky bits on ISO images.

Ah thanks, missed that.

Shouldn't the masks allow to clear the non-ACCESSPERMS bits? Or are you considering that redundant with, e.g., the nosuid mount option?

So I do think that nosuid makes suid and guid masks redundant. Being an inherently read-only filesystem, masking off the sticky bit isn't really useful either. dirmask and fmask having the same semantics across filesystems is also ideal.

OTOH, if we had any writable filesystems for which the sticky bit was relevant, I do think it would be simpler and more flexible if the masks applied to ALLPERMS instead of just ACCESSPERMS. msdosfs wouldn't change its defacto semantics if we made that change since it doesn't support those bits anyway.

[olce]

In D46948#1075027, @jhb wrote:

OTOH, if we had any writable filesystems for which the sticky bit was relevant, I do think it would be simpler and more flexible if the masks applied to ALLPERMS instead of just ACCESSPERMS. msdosfs wouldn't change its defacto semantics if we made that change since it doesn't support those bits anyway.

I was also thinking along this line. I agree with both of you that in this case it doesn't seem to matter much, but consistency is also important and accommodating for future R/W filesystems (or the addition of masks to existing ones) tends to push towards just applying the mask to ALLPERMS. For the sticky bit, as you said, but perhaps also to be able to selectively allow only one the setuid and setgid bits (an effect which could be obtained alternatively through new mount options).

If keeping the current behavior, I would recommend ensuring that the mount fails if non-supported bits in the mask are specified. That way, an extension to more bits would still be possible in the future without breaking compatibility.

[jhb]

In D46948#1075055, @olce wrote:

In D46948#1075027, @jhb wrote:

OTOH, if we had any writable filesystems for which the sticky bit was relevant, I do think it would be simpler and more flexible if the masks applied to ALLPERMS instead of just ACCESSPERMS. msdosfs wouldn't change its defacto semantics if we made that change since it doesn't support those bits anyway.

I was also thinking along this line. I agree with both of you that in this case it doesn't seem to matter much, but consistency is also important and accommodating for future R/W filesystems (or the addition of masks to existing ones) tends to push towards just applying the mask to ALLPERMS. For the sticky bit, as you said, but perhaps also to be able to selectively allow only one the setuid and setgid bits (an effect which could be obtained alternatively through new mount options).

If keeping the current behavior, I would recommend ensuring that the mount fails if non-supported bits in the mask are specified. That way, an extension to more bits would still be possible in the future without breaking compatibility.

The commit to cd9660 is only in main, so we have time to fix it to be the semantics we want now without worrying about compatibility. I think we should either do this change or switch to having the mask apply to ALLPERMS, but I don't see a reason to keep the current behavior in main.

[jhb]

I've just posted an alternate version that expands the masks to all 12 bits at D47357.