Bug 307431

Adding GFS2 to summary so it shows up in Steve Whitehouse's lists.

See also #242066 (there isn't a see also field, so I have to add a dep one way
or the other).

It looks like we need xattr_permission, but that isn't exported at the moment.

Reassigning to Ryan since this was his contribution in the first place.

Looked through the code and Steve (comment #) is correct. It appears
xattr_permission is what we want. Not sure why it is not exported. Looking into it.

Actually .. I retract my previous comment. I don't think xattr_permission is
what we want. Appears that xattr_permission places no restrictions on selinux
xattrs. security_inode_getxattr and security_inode_setxattr seem to be needed,
as Eric stated.

That said, we'll probably get proper handling/permission checks for free if we
use the generic xattr interfaces. That may be the best solution for GFS2, but
I'm not sure how we'll go about solving this for GFS1.

There are two issues here. In the code where permission() is currently being
(incorrectly) used it is the generic code which redoes the permission checks
after we got our glocks. Thats why xattr_permission isn't exported - no other
filesystem requires this and we'll need to argue that point before we'll be
allowed to export that function I'm sure.

As you say, xattr_permission doesn't do any checking if the xattr type is
"security". Thats correct, as the security handlers themselves should be doing
that, in this case with security_inode_get/setxattr as per the title of the bug.

Although we really need to redo the xattr code to use more of the VFS provided
library functions (and also to allow us to merge transactions during inode
creation and deletion) it should be possible to make these changes (for 5.2)
without changing a lot of code.

Having has a quick look at the VFS functions, I'd expect that when we do get
around to the rewrite, it will save us about 1000 lines of code compared with
the current implementation, plus (and more importantly) save us from extra
transactions when creating and deleting inodes. Thats all under bz #242066 though.

Would like to know how to recreate/test this problem so that I can verify the
fix. Any information would be helpful.

Setup an SELinux box in enforcing mode.  Which say apache using gfs2 file system
to display its pages.

Directories need to be labeled 
httpd_sys_content_t

chcon -R -t httpd_sys_content_t /PATHTOGFS2

Try to look at the web site.

is that right dan?  is there a transition to chcon_t or something from
unconfined_t?  I'm reasonably certain there is a transition from unconfined_t to
restorecon_t.  Remember the problem is not with actual usage of GFS, but with
getting/setting the xattrs.

maybe a better test would be

mkdir -o /opt/httpd/test1dir/test2dir
semanage fcontext -a -t httpd_sys_content_t "/opt/httpd(/.*)?"
restorecon -R -v /opt/

if you get any selinux denials its not right.  If you can then ls -lZ /opt/httpd
and see the new httpd_sys_content_t on those subdirs you should be good.

Yes that is good, but I also want to make sure confined domains can read the
privs.  restorecon is a fairly privledged application.

Verified this bug with procedure described above. GFS2 was unable to relabel
files/directories due to the permission() calls occuring in the
gfs2_security_eaops (permission was returning -EACCESS). Ripped our permission
check and restorecon worked properly. Appears that this is the fix. No need to
cal security_inode_setxattr, etc. since this in done in the vfs layer (ie.
vfs_setxattr).

What is the correct thing to do for the other xattr types? As far as I can tell,
we don't need to call permission() in any of the xattr operations.

Comments, please.

I think you are right in that you don't need your own permission hooks inside
the FS.

If you gett a running machine with the perms ripped out can I get access?   I'm
not sure a good way to test to make sure the security hooks are getting hit
correctly, but i'm sure with enough playing I can verify (at least the selinux
xattrs)

OK. I am going to rip out all the permission() calls in each of the xattr ops.
It looks like xattr_permission() is doing the correct checks anyway. Going to
send as a single patch.

Created attachment 271791 [details]
Remove permission checks from xattr ops

Patch against RHEL5 branch.

Patch went upstream (Steve's git tree) and to rhkernel-list. Attached patch
against RHEL5 kernel (above).

Movied status to POST.

in 2.6.18-61.el5
You can download this test kernel from http://people.redhat.com/dzickus/el5

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on the solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2008-0314.html