Date: Fri, 21 Jun 2019 19:41:15 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Alan Somers <asomers@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r349248 - in head/sys: fs/fifofs kern sys Message-ID: <20190621185323.C1023@besplex.bde.org> In-Reply-To: <201906202307.x5KN7LLB002088@repo.freebsd.org> References: <201906202307.x5KN7LLB002088@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 20 Jun 2019, Alan Somers wrote:
> Log:
> fcntl: fix overflow when setting F_READAHEAD
>
> VOP_READ and VOP_WRITE take the seqcount in blocks in a 16-bit field.
> However, fcntl allows you to set the seqcount in bytes to any nonnegative
> 31-bit value. The result can be a 16-bit overflow, which will be
> sign-extended in functions like ffs_read. Fix this by sanitizing the
> argument in kern_fcntl. As a matter of policy, limit to IO_SEQMAX rather
> than INT16_MAX.
> ...
> Modified: head/sys/kern/vfs_vnops.c
> ==============================================================================
> --- head/sys/kern/vfs_vnops.c Thu Jun 20 22:21:42 2019 (r349247)
> +++ head/sys/kern/vfs_vnops.c Thu Jun 20 23:07:20 2019 (r349248)
> @@ -499,7 +499,8 @@ sequential_heuristic(struct uio *uio, struct file *fp)
> * closely related to the best I/O size for real disks than
> * to any block size used by software.
> */
> - fp->f_seqcount += howmany(uio->uio_resid, 16384);
> + fp->f_seqcount += MIN(IO_SEQMAX,
> + howmany(uio->uio_resid, 16384));
> if (fp->f_seqcount > IO_SEQMAX)
> fp->f_seqcount = IO_SEQMAX;
This already limited the result to IO_SEQMAX. Now it limits the result to
IO_SEQMAX twice.
Using MIN() is a style bug. 4.4BSD removed the MIN() macro from the kernel
and replaced it by *min() inline functions. MIN() was restored because
too much contribed code used it, but it should not be used in core code.
However, *min() is not type-generic, so it is hard to use. Using
f_seqcount = imin(IO_SEQMAX, howmany(uio->uio_resid, 16384));
would restore the overflow bug. This code worked originally when uio_resid
had type int. It was broken by changing uio_resid's type to ssize_t.
This makes howmany(uio->uio_resid, 16384) have type ssize_t too, and its
value overflows on assignment to "int f_seqcount" before the value is
clamped to IO_SEQMAX, whenever uio_resid is especially preposterously
large (2G * 16384) on 64-bit systems which pretend to support such silly
i/o sizes.
imin() is as bad as the blind assignment to an int, since its prototype
cause blind conversion to int. I have some macros for *min() which
optionally detect such type errors or just do much the same as MIN()
(but using inline functions), but these are not quite of production
quality. The case where one arg has a signed type and the other arg
has a signed type is hardest to handle. This has sign extension or
unsign extension bugs in general. MIN() blindly does the extension.
The overflow usually only causes negative f_seqcount which should be
treated as 0.
> return (fp->f_seqcount << IO_SEQSHIFT);
>
> Modified: head/sys/sys/file.h
> ==============================================================================
> --- head/sys/sys/file.h Thu Jun 20 22:21:42 2019 (r349247)
> +++ head/sys/sys/file.h Thu Jun 20 23:07:20 2019 (r349248)
> @@ -179,7 +179,10 @@ struct file {
> /*
> * DTYPE_VNODE specific fields.
> */
> - int f_seqcount; /* (a) Count of sequential accesses. */
> + union {
> + int16_t f_seqcount; /* (a) Count of sequential accesses. */
> + int f_pipegen;
> + };
> off_t f_nextoff; /* next expected read/write offset. */
> union {
> struct cdev_privdata *fvn_cdevpriv;
f_seqcount should still have type int. Conversion of any type smaller than
int to int just wastes time, and int16_t is a bogus type for holding values
limited to 0x7F.
int for f_seqcount costs no space, since f_pipegen still has the correct
type and the union is padded to at least the size of its largest member.
This struct has delicate ordering which usually minimizes padding,
although it violates style(9) for most integer members. Before
f_seqcount, there are some pointers, then 2 shorts and 2 u_int's, so
64-bit alignment occurs after f_seqcount on 64-bit arches, just in
time for there to be no padding before the 64-bit f_offset. style(9)
requires sorting all the 64-bit types first.
My macros look like:
XX #define __min(x, y) \
XX ( \
XX (sizeof(x) == 8 || sizeof(y) == 8) ? \
XX ((__typeof(x))-1 == -1 && (__typeof(y))-1 == -1) ? \
XX _qmin((x), (y)) \
XX : \
XX _uqmin((x), (y)) \
XX : \
XX ((__typeof(x))-1 == -1 && (__typeof(y))-1 == -1) ? \
XX _imin((x), (y)) \
XX : \
XX _min((x), (y)) \
XX )
XX
XX #ifdef MACRO_MIN
XX static __inline int _imin(int a, int b) { return (a < b ? a : b); }
XX ...
XX #define imin(a, b) __min((a), (b))
XX #else
XX #ifdef __GNUC__
XX static __inline int imin(int a, int b) { return (a < b ? a : b); }
XX #endif
XX #endif
One reason that these are not production quality is that they require
gnu __typeof() but production quality must work with any C compiler.
This worked with gcc-1 or 2. Later versions of gcc have better support
for type-generic functions, but that is even more unportable.
These can be used to find type errors: compile without and with MACRO_MIN.
If the object code changes, then the non-underscored function must have
had wrong types since the type-generic macro selected a sub-version with
different types.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190621185323.C1023>