cvs commit: src/sys/ufs/ufs ufs_quota.c
mpp at mail.mppsystems.com
Fri Feb 2 01:06:34 UTC 2007
On Fri, Feb 02, 2007 at 03:34:15AM +1100, Bruce Evans wrote:
> On Thu, 1 Feb 2007, Mike Pritchard wrote:
> >mpp 2007-02-01 01:01:57 UTC
> > FreeBSD src repository
> > Modified files:
> > sys/ufs/ufs ufs_quota.c
> > Log:
> > Disallow negative UIDs when processing quotactl options.
> Er, uids are unsigned, so they cannot be negative.
There are 1 or 2 utilities out there that will display a
uid/gid of UINT_MAX-2 as -2 :)
> The function actually takes a u_long id and now uses a bogus cast
> ((int)id) to check for "negative" values. The correct check is
> something like (id <= QUOTA_ID_MAX). ((int)id) would work to restrict
> the id to <= INT_MAX due to previous bogusness (*), but I don't see
> the point of that. If ints are 32-bits then id = INT_MAX gives an
> offset that is about half as huge as id = UINT_MAX (64G?), and if
> ints are 64 bits then id = INT_MAX and id = UINT_MAX both give
> physically impossible offsets. Is the problem with negative ids
> mainly that they are standard for nfs without maproot?
The basic problem is that the quota file will grow in size
to sizeof(struct dqblk) * highest_id. sizeof(struct dqblk) = 32.
At system startup quotacheck has to read the entire file,
which if the highest_id is extremely large (as a file copied from
an nfs file system without maproot might have, or from some
type of archive file that may have the id = -2)
The data file was also being incorrectly truncated to a maximum size of
2^32 bytes due to some incorrect casting when writing out the dqblock data.
A high id value of 2^24 (16.7 million) allows quotacheck to run
in a "reasonable" (1 min per quota file) amount of time at system startup.
An high id value of 2^25 (33 million) bumps it up to 2 mins per quota file.
I'd be happy to change the code to use a new QUOTA_ID_MAX value.
> (*) The id started as "int id" in the user API, but got punned in
> several stages to "uid_t uid" in the kernel quotactl() and
> vfs_quotactl_t. ufs_quotactl() now starts by punning it half way back
> (uid_t id). As a result of this, any overflow for converting uids and
> gids to ids has already occurred at the syscall boundary, and most
> overflows in the kernel consist of overflowing back to the original
> value (uid_t -> int -> uid_t kernel) and back to the already-overflowed
> value (uid_t -> int -> uid_t -> int). Since uid_t is uint32_t and int
> is 32 bits 2's complement on all supported systems, the only overflows
> of ids that occur now are from huge positive to negative and back.
> However, if the id is initially -1, then all overflows are in the
> kernel (-1 -> u/gid -> ...). Also, an u/gid 0xffffffff overflows to
> -1 at the syscall boundary and then gets corrrupted to the real u/gid
> in the kernel.
> The user API is only correct if ints have more bits that u/gids, so
> that conversion of u/gids to ints doesn't overflow and -1 isn't in-band.
> It was correct before 4.4BSD on systems with >= 17-bit ints, since
> u/gid_t was only 16 bits until then.
> The patch has some style bugs (misformatted comment, unlike the other
> XXX comments about "negative" uids).
Found it...will get it fixed.
mpp @ mppsystems.com or mpp @ FreeBSD.org
"If tyranny and oppression come to this land, it will be in the guise
of fighting a foreign enemy." - James Madison (1787)
More information about the cvs-src