nfs_clsubs.c: Fix ncl_getcookie() when "pos" is negative

[rmacklem]

Summary

When the "pos" variable in ncl_getcookie() is negative,
it can skip past the while (pos >= NFSNUMCOOKIES) loop
and return a bogus pointer instead of NULL.

This patch declares "pos" as a u_int to avoid the problem.

Test Plan

Tested using a small program that open()s a large directory,
reads the first block and then lseek()s to a large offset, such
that "pos" was calculated as negative.

[markj]

sys/fs/nfsclient/nfs_clsubs.c

277

We're potentially truncating some bits in off here. Is there any reason pos shouldn't be a uoff_t?

[rmacklem]

Add a sanity limit for "off" to avoid an
overflow when calculating "pos".
This ensures a NULL reply for overly
large offsets.

[rmacklem]

sys/fs/nfsclient/nfs_clsubs.c

277

I've added a sanity check to ensure there
is no overflow. Either change deals with the
overflow, but for the case where a very large
directory is being read from the NFS server,
I think even doing 1billion / 31 mallocs is
pushing the limits of what a system might
handle. (There has generally been the assumption
that the NFS client should try to handle whatever
the NFS server throws at it and, if it gets hung, the
user/admin will know not to mount that server again.
However, 1billion 8K directory blocks seems more that
adequate?)

[markj]

sys/fs/nfsclient/nfs_clsubs.c

277

But you're limiting the number of mallocs here to 1billion / NFS_DIRBLKSIZE / 31 if I'm not mistaken.

For my original question, I think pos does need to be a 32-bit integer at least so long as ndm_eocookie is 32 bits wide. But we could widen that field too. Either way, I think it should be made unsigned, like pos is.

[rmacklem]

Increase the "off" sanity limit and declare ndm_eocookie
as u_int as suggested by markj@.

[rmacklem]

sys/fs/nfsclient/nfs_clsubs.c

277

Yea, you were right. A struct dirent for a long filename
takes 512bytes, so 1Gbyte holds 2million directory
entries. (I'm not sure a directory with more than
2million filenames in it makes sense, but..)

I've increased the sanity limit to 50Gbytes, which will
handle 100million entries for long filenames.

[markj]

sys/fs/nfsclient/nfs_clsubs.c

272

Assuming that "overflow" here refers to the possibility that the value of (uoff_t)off / NFS_DIRBLKSIZ will be truncated when stored in pos, I'd suggest writing "This limit ensures that "off" will not be truncated in the division below" instead.