fcntl: fix overflow when setting F_READAHEAD

[asomers]

Summary

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.

Test Plan

Tested with a simple C program and a dtrace script. The C program sets
F_READAHEAD to various values and then reads a file. The dtrace script
prints the value of seqcount at the entrance to ffs_read.

seqcount.c

#include <err.h>
#include <fcntl.h>
#include <stdlib.h>
#include <unistd.h>

int main(int argc, char **argv)
{

const char *filename = argv[1];
int seqcount = atoi(argv[2]);
int fd;
void *buf;

buf = malloc(65536);

fd = open(filename, O_RDONLY);
if (fd < 0)
        err(1, "open");

if (fcntl(fd, F_READAHEAD, seqcount) < 0)
        err(1, "fcntl");

if (read(fd, buf, sizeof(buf)) < 0)
        err(1, "read");

return (0);

}

invocation:

sudo dtrace -i 'fbt:kernel:ffs_read:entry {printf("seqcount=%d", args[0]->a_ioflag >> 16);}' -c "./seqcount sparse 2147483647"

[cem]

Looks like you could get a similar overflow by having 64k readers on a pipe too (fs/fifofs/fifo_vnops.c). You could also overflow in sequential_heuristic() if uio_resid is big enough (especially with a narrower f_seqcount).

sys/kern/kern_descrip.c

785 ↗

(On Diff #58838)

style(9): 4 space indent on trailing lines

sys/sys/file.h

182 ↗

(On Diff #58838)

I'd prefer explicit int16_t, although it looks like it could actually be int8_t given IO_SEQMAX is 0x7f.

[cem]

Also: nice find! :)

[asomers]

Fix an overflow in sequential_heuristic introduced by reducing the size of
f_seqcount.

[asomers]

Actually, I don't think the usage in fifo_vnops.c is a potential overflow. AFAICT fifos use that field for a completely different purpose. It really should be a different field (and in fact, it used to be). See r238936.

sys/sys/file.h

182 ↗

(On Diff #58838)

Ok. But I don't want to use int8_t because we're still storing this field in the upper 16 bits of ioflag, so I think it's more natural to use 16 bits in the file structure as well.

[cem]

In D20710#447668, @asomers wrote:

Actually, I don't think the usage in fifo_vnops.c is a potential overflow. AFAICT fifos use that field for a completely different purpose. It really should be a different field (and in fact, it used to be). See r238936.

Interesting, thanks. It still seems like it could overflow, especially if the f_seqcount value is shrunk past int — it's later compared against the pipe's wgen, which is an int. We could instead do something like:

union {
    intXX_t f_seqcount;
    int f_pipegen
};

That way it is clear the repurposing is intentional and correctly sized for fifofs' use.

sys/sys/file.h

182 ↗

(On Diff #58838)

We're storing the value in bits 16-22 of ioflag (some middle 7 bits), not the upper 16, due to clearly intentional IO_SEQMAX. I don't feel strongly about int8_t over int, but I don't think a 16-bit type is the obvious natural type for the stored value.

[asomers]

Use 8 bits for seqcount and 32 for pipegen

[cem]

Need to change fifofs to actually use f_pipegen instead of f_seqcount :)

[asomers]

Add missing files to review

[cem]

Thanks!

[asomers]

FYI, I discovered that f_seqcount needs to be int16_t rather than int8_t in order to detect an overflow. Otherwise the slightest overflow of IO_SEQMAX will wrap.

[cem]

In D20710#447768, @asomers wrote:

FYI, I discovered that f_seqcount needs to be int16_t rather than int8_t in order to detect an overflow. Otherwise the slightest overflow of IO_SEQMAX will wrap.

Wrap where? If we can overflow at 8 bits, the margin for overflow at 16 isn't great (at most an extra factor of 256x). That condition may need to be fixed either way.

[asomers]

Wrap in vfs_vnops.c at line 502. Currently, the largest possible overflow is about 2x. So using an int16_t makes it safe.

[cem]

In D20710#447771, @asomers wrote:

Wrap in vfs_vnops.c at line 502. Currently, the largest possible overflow is about 2x. So using an int16_t makes it safe.

Oh, I see, the expression is just incorrect. Should be something like:

fp->f_seqcount = MIN(IO_SEQMAX,
    fp->f_seqcount + howmany(uio->uio_resid, 16384));

(The type of uio_resid is ssize_t, so that expression should be evaluated as ssize_t and not overflow before truncation to IO_SEQMAX.)

The fact that the current code violates IO_SEQMAX is a problem even if it no longer wraps with int16_t, I think.