Re: [patch 12/12] vfs: allow file truncations when both suid and write permissions set

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:

> Sorry for same argument. I see. However, um...
>
> I found this piece in security/selinux/hooks.c
>
> static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
> {
> 	const struct cred *cred = current_cred();
>
> 	if (iattr->ia_valid & ATTR_FORCE)
> 		return 0;
>
> 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
> 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
> 		return dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
>
> 	return dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
> }
>
> I guess it's assuming the ia_valid doesn't have (ATTR_MODE | ATTR_SIZE),
> but truncate() already does it, I don't know whether it's ok. The
> definition of ATTR_FORCE is unclear at all, it would be problem. But,
> I'm not sure though, I suspect the above code also has problem...

I found the SuSv3 says for ftruncate():

Upon successful completion, if fildes refers to a regular file,
ftruncate() shall mark for update the last data modification and last
file status change timestamps of the file and the S_ISUID and S_ISGID
bits of the file mode may be cleared. If the ftruncate() function is
unsuccessful, the file is unaffected.

And vmtruncate() can return error easily with RLIMIT_FSIZE or
->s_maxbytes. So, I think clearing s[ug]id first may be bad behavior
without good reason.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>