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)
Mon, Apr 8, 10:15 PM
Unknown Object (File)
Mar 19 2024, 3:20 AM
Unknown Object (File)
Mar 12 2024, 9:48 AM
Unknown Object (File)
Mar 12 2024, 9:48 AM
Unknown Object (File)
Mar 12 2024, 9:48 AM
Unknown Object (File)
Mar 12 2024, 9:48 AM
Unknown Object (File)
Mar 12 2024, 9:47 AM
Unknown Object (File)
Mar 12 2024, 9:47 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 Not Applicable
Unit
Tests Not Applicable

Event Timeline

sys/fs/nfsserver/nfs_nfsdport.c
3075 ↗(On Diff #72496)

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 ↗(On Diff #72590)

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 ↗(On Diff #72590)

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 ↗(On Diff #72590)

This constant can be spelled as UID_NOBODY.

1490 ↗(On Diff #72590)

And this one is GID_NOGROUP.

3424 ↗(On Diff #72590)

UID_NOBODY

3425 ↗(On Diff #72590)

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 ↗(On Diff #72590)

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 ↗(On Diff #72590)

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 ↗(On Diff #72590)

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

1490 ↗(On Diff #72590)

Ditto above.

sys/fs/cd9660/cd9660_vfsops.c
104 ↗(On Diff #72590)

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 ↗(On Diff #72590)

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 ↗(On Diff #72590)

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