Date: Fri, 2 Feb 2007 03:34:15 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Mike Pritchard <mpp@freebsd.org> Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/ufs/ufs ufs_quota.c Message-ID: <20070202015415.G924@delplex.bde.org> In-Reply-To: <200702010101.l1111v4H029618@repoman.freebsd.org> References: <200702010101.l1111v4H029618@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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. 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 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). Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070202015415.G924>