Page MenuHomeFreeBSD

Ignore F_SETLK_REMOTE requests for sysid 0.
ClosedPublic

Authored by markj on Mar 25 2019, 4:32 PM.

Details

Summary

sysid 0 is the local system. Without this check, it's possible to
trigger the KASSERT in lf_clearremotesys(). Note that F_SETLK_REMOTE
is only available to privileged users.

I removed the comment about a temporary API because it's been in FreeBSD
for over 10 years and is used by some tests.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj created this revision.Mar 25 2019, 4:32 PM
markj added a reviewer: kib.Mar 25 2019, 4:33 PM
kib added a comment.Mar 25 2019, 4:57 PM

Is there a check that F_REMOTE has l_sysid != 0 ? I cannot find it, if any. We require PRIV_NFS_LOCKD for it, but I think slightly less trust would be due.

markj added a comment.Mar 25 2019, 7:26 PM
In D19702#422050, @kib wrote:

Is there a check that F_REMOTE has l_sysid != 0 ? I cannot find it, if any. We require PRIV_NFS_LOCKD for it, but I think slightly less trust would be due.

You mean, in the lockf layer? I don't think so. I don't quite understand why NLM is setting l_sysid = 0 in nlm_getlock().

kib added a comment.Mar 25 2019, 8:11 PM
In D19702#422050, @kib wrote:

Is there a check that F_REMOTE has l_sysid != 0 ? I cannot find it, if any. We require PRIV_NFS_LOCKD for it, but I think slightly less trust would be due.

You mean, in the lockf layer? I don't think so.

I mean, when fcntl(F_SETLK_REMOTE) in kern_descrip.c

I don't quite understand why NLM is setting l_sysid = 0 in nlm_getlock().

It seems to only do that for F_GETLK implementation, where userspace does not need to see l_sysid anyway.

markj updated this revision to Diff 55438.Mar 25 2019, 8:19 PM

Make the check more strict: disallow sysid == 0 for any F_REMOTE request.

kib added inline comments.Mar 25 2019, 8:26 PM
sys/kern/kern_descrip.c
675 ↗(On Diff #55438)

But now this version lost your original fix for F_UNLCKSYS ?

markj added inline comments.Mar 25 2019, 8:29 PM
sys/kern/kern_descrip.c
675 ↗(On Diff #55438)

The new check catches this case too. We perform the check for all verbs instead of just F_UNLCKSYS.

kib accepted this revision.Mar 25 2019, 8:46 PM
This revision is now accepted and ready to land.Mar 25 2019, 8:46 PM
This revision was automatically updated to reflect the committed changes.