ffs: Clamp BIO_SPEEDUP length
ClosedPublic
Actions

Authored by markj on Nov 3 2020, 8:56 PM.

Details

Reviewers

kib
mckusick
imp
chs

Commits

rS367589: ffs: Clamp BIO_SPEEDUP length

Summary

On 32-bit platforms, the computed size of the BIO_SPEEDUP submitted by
softdep_request_cleanup() may be negative when assigned to bp->b_bcount,
which has type "long".

Clamp the size to LONG_MAX. We could perhaps limit it to IOSIZE_MAX,
but that would reduce the limit to 2GB on 64-bit platforms, and I'm not
sure if that's desirable.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 34612
Build 31701: arc lint + arc unit

Event Timeline

markj requested review of this revision.Nov 3 2020, 8:56 PM

markj created this revision.

Harbormaster completed remote builds in B34610: Diff 79147.Nov 3 2020, 8:56 PM

markj added reviewers: kib, mckusick, imp, chs.Nov 3 2020, 8:56 PM

kib accepted this revision.Nov 3 2020, 9:13 PM

This revision is now accepted and ready to land.Nov 3 2020, 9:13 PM

kib added inline comments.Nov 3 2020, 9:16 PM

sys/ufs/ffs/ffs_softdep.c
1467	Wouldn't better type for shortage be ufs2_daddr_t or even off_t ?

markj added inline comments.Nov 3 2020, 9:27 PM

sys/ufs/ffs/ffs_softdep.c
1467	I thought about using ufs2_daddr_t since that's the type used by the caller, but it seemed kind of strange to me. off_t seems more reasonable.

The same issue exists in g_io_speedup() (though that function appears to be unused at the moment).

imp added inline comments.Nov 3 2020, 9:33 PM

sys/ufs/ffs/ffs_softdep.c
1477	don't we have a min variant we can use here? min(shortage, LONG_MAX) ????

I'd have thought that b_bcount would have been a size_t or off_t too like bio_length is, not a long too....
Maybe we should kassert b_bcount >= 0 when we submit to strategy since it is weirdly signed.

Use off_t.
Use omin().

This revision now requires review to proceed.Nov 3 2020, 9:42 PM

Harbormaster completed remote builds in B34612: Diff 79150.Nov 3 2020, 9:43 PM

Update g_io_speedup() as well.

In D27081#604389, @chs wrote:

The same issue exists in g_io_speedup() (though that function appears to be unused at the moment).

It's not quite the same since there's no sign extension happening, I believe.

Harbormaster completed remote builds in B34613: Diff 79152.Nov 3 2020, 9:50 PM

I wonder if we can hit this issue inside of ffs_blkfree as well, since we might free > 2GB in one shot on i386 as well...

In D27081#604410, @imp wrote:

I wonder if we can hit this issue inside of ffs_blkfree as well, since we might free > 2GB in one shot on i386 as well...

Could be. In most places it is used to free a single block. I suspect that the FFS_BLK_FREE sysctl handler can misbehave on 32-bit systems.

emaste added a subscriber: emaste.Nov 10 2020, 3:26 PM

This revision was not accepted when it landed; it landed in state Needs Review.Nov 11 2020, 1:48 PM

Closed by commit rS367589: ffs: Clamp BIO_SPEEDUP length (authored by markj). · Explain Why

This revision was automatically updated to reflect the committed changes.

markj added a commit: rS367589: ffs: Clamp BIO_SPEEDUP length.