Page MenuHomeFreeBSD

fix sanity check for offset and length of Allocate NFSv4.2 operation
ClosedPublic

Authored by rmacklem on Aug 12 2021, 2:20 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 20 2024, 2:46 PM
Unknown Object (File)
Jan 22 2024, 10:36 AM
Unknown Object (File)
Jan 4 2024, 1:12 PM
Unknown Object (File)
Dec 20 2023, 5:51 AM
Unknown Object (File)
Dec 1 2023, 5:14 AM
Unknown Object (File)
Oct 15 2023, 4:01 AM
Unknown Object (File)
Oct 7 2023, 11:53 PM
Unknown Object (File)
Sep 29 2023, 2:32 AM
Subscribers
None

Details

Summary

When coding the Deallocate operation, I spotted that the sanity
check in Allocate looks bogus.
off and len are off_t (signed int64_t)
lo_first and lo_end are unsigned uint64_t

I added a check for off < 0 and used lo_first
to ensure the addition is done unsigned.
I check that lo_end does not exceed OFF_MAX,
which I think is as large as any FreeBSD file system
can handle.

Does this version look correct?
(I could also do the lo_end < lo_first check, but an
overflow could only occur if off_t became uint64_t.
Is that ever likely to happen?)

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rmacklem created this revision.
sys/fs/nfsserver/nfs_nfsdserv.c
5361

Don't you still need to check that lo_end > lo_first?

sys/fs/nfsserver/nfs_nfsdserv.c
5361

Well, since off and len are both checked non-negative, that
means they can't be greater than OFF_MAX.
--> OFF_MAX + OFFMAX cannot be greater than UINT64_MAX.
So, as long as off_t is signed, lo_end could not have overflowed.
--> lo_end could be less than lo_first only if off or len were negative,

I think?

I can add lo_end > lo_first back in as a safety belt in case off_t is
changed to unit64_t or int128_t someday.

Added lo_end < lo_first as suggested by kib@.

This revision is now accepted and ready to land.Aug 12 2021, 3:06 PM