Page MenuHomeFreeBSD

fix export_args flags field to be 64bits and handle more than 16 groups for the mapped user
ClosedPublic

Authored by rmacklem on Jun 1 2020, 3:21 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 19, 1:08 AM
Unknown Object (File)
Tue, Nov 19, 1:08 AM
Unknown Object (File)
Tue, Nov 19, 1:08 AM
Unknown Object (File)
Tue, Nov 19, 1:08 AM
Unknown Object (File)
Tue, Nov 19, 1:08 AM
Unknown Object (File)
Tue, Nov 19, 1:08 AM
Unknown Object (File)
Mon, Nov 18, 1:05 PM
Unknown Object (File)
Sun, Nov 17, 10:32 AM
Subscribers

Details

Summary

Since mnt_flags was upgraded to 64bits there has been a quirk in "struct export_args", since
it holds a copy of mnt_flags in ex_flags, which is an "int" (32bits).
This happens to currently work, since all the flag bits used in ex_flags are defined in the low
order 32bits. However, new export flags cannot be defined.
Also, ex_anon is a "struct xucred", which limits it to 16 additional groups.

This patch revises "struct export_args" to make ex_flags 64bits and replaces ex_anon with
ex_uid, ex_ngroups and ex_groups (which points to a groups list, so it can be malloc'd up to NGROUPS
in size.

This requires that the VFS_CHECKEXP() arguments change, so I also modified the last "secflavors"
argument to be an array pointer, so that the secflavors could be copied in VFS_CHECKEXP() while
the export entry is locked. (Without this patch VFS_CHECKEXP() returns a pointer to the secflavors
array and then it is used after being unlocked, which is potentially a problem if the exports entry
is changed. In practice this does not occur when mountd is run with "-S", but I think it is worth fixing.)

For mountd.c, I replaced "struct xucred" with a locally defined "struct expcred" that handles NGROUPS
groups. As coded in this patch, this does make the "exportlist" and "grouplist" structures about 4K larger,
but keeps the patch pretty straightforward.
I have also coded a version that has a small 16 entry groups list and then malloc's an array for larger
groups lists. This keeps the above structures from growing, but makes the code more complex
(and I'm not sure I got it right;-) because it must free() the malloc'd group arrays at the correct places.
If you think doing this is preferable, I can pursue this variant of the mountd patch.

Test Plan

I have done moderate testing with exports files using both old and patched mountds.
I will be continuing with testing of more exports file variants.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 31453

Event Timeline

sys/fs/nfsserver/nfs_nfsdport.c
3073–3074

We should be able to avoid the extra copies now

Deleted the copying of secflavors in nfsvno_checkexp() and
nfsvno_fhtovp() as suggested by freqlabs@.

sys/fs/cd9660/cd9660_vfsops.c
104

Why do you left use o2export_args for in-tree fs ?

I understand that o2export_args is present to provide compatibility with old mountd, but other uses in kernel just leave a feel of unfinished conversion. Also I suspect you would be able to remove some code from vfs_mount.c.

sys/fs/nfsserver/nfs_nfsdport.c
3708

Shouldn't the truncation of the number of the aux groups be an error instead of silent truncation (there and in all other places) ? Typically it would cause user to not get some permissions, but sometimes it would cause user to get undeserved access.

usr.sbin/mountd/mountd.c
1488

This constant can be spelled as UID_NOBODY.

1490

And this one is GID_NOGROUP.

3424

UID_NOBODY

3425

GID_NOGROUP

Replied to inline comments.

The main question is "what to do with vfs_oexport_conv().
I think just getting rid of it is the preferred option, since
having a "userspace" vs "sysspace" flag in export_args
seems awkward and it isn't needed anyhow.

sys/fs/cd9660/cd9660_vfsops.c
104

Well, doing it for exports_args is trickier than it looks.
ex_groups in the new one is just a pointer, which with
the patch as it is now, always points to userspace and
is used by copyin().

I could add a "ex_seg" field to the new export_args to
indicate that cr_groups is in sysspace, and then memcpy()
it, but what if it is bogus. (ie. I don't know how to validate
the pointer to ensure it won't crash when in sysspace?)
If you tell me how to validate it for sysspace, then I could
make that change.

Or we could just get rid of vfs_oexport_conv(), since the
code in vfs_domount_update() knows how to handle the
"struct oexport_args" and conversion isn't really necessary
anyhow.

sys/fs/nfsserver/nfs_nfsdport.c
3708

Yea, I can do that.
The old code (as in head) doesn't even sanity check the
field and just assumed mountd got it right.
(The syscall can only be done by root, so it isn't a
security hole.)

I'll change it to an error.

usr.sbin/mountd/mountd.c
1488

In conf.h, this is #ifdef _KERNEL and I was afraid of
exposing it to userland.

1490

Ditto above.

sys/fs/cd9660/cd9660_vfsops.c
104

My intuitive reaction is that vfs_mount.c must handle copying of ex_groups, and all code in kernel except the code that does uio args copyin, should get plain kernel pointer.

I am not sure how hard this would be.

usr.sbin/mountd/mountd.c
1488

sys/conf.h is not popular header among userspace. I think we can move all UIDs/GIDs definitions from under _KERNEL, or to have them protected by e.g.

#if defined(_KERNEL) || defined(_WANT_UID)

Let handle it after this patch lands as is.

Got rid of vfs_oexport_conv(), since do_mount_update()
knows how to handle/convert struct oexport_args, so
conversion in the vfs_cmount() calls is not necessary.

This results in the copyin() of groups for the new struct export_args
only being done in do_mount_update() and the nfssvc() syscall
for the V4 root exports.

I also repaced uses of NGROUPS with NGROUPS_MAX + 1, to
avoid the assumption that NGROUPS == NGROUPS_MAX + 1.

rmacklem marked an inline comment as done.

Oops, missed one change in the last patch update.

Marked inline comments as done, except for using UID_NOBODY, GID_NOGROUP.
These will be done as a separate patch, along with moving the definitions of them
outside #ifdef _KERNEL in sys/conf.h, as suggested by kib@.

sys/fs/cd9660/cd9660_vfsops.c
104

Removal of vfs_oexport_conv() allows the copyin() to
only be done in do_mount_update().
(It is done in the NFS server code for nfssvc(), but that

is a separate syscall.)
This revision is now accepted and ready to land.Jun 13 2020, 10:22 PM