Page MenuHomeFreeBSD

ffs: Clamp BIO_SPEEDUP length
ClosedPublic

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

Details

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

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj requested review of this revision.Nov 3 2020, 8:56 PM
markj created this revision.
This revision is now accepted and ready to land.Nov 3 2020, 9:13 PM
sys/ufs/ffs/ffs_softdep.c
1467 ↗(On Diff #79147)

Wouldn't better type for shortage be ufs2_daddr_t or even off_t ?

sys/ufs/ffs/ffs_softdep.c
1467 ↗(On Diff #79147)

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).

sys/ufs/ffs/ffs_softdep.c
1477 ↗(On Diff #79147)

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
markj marked 2 inline comments as done.

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.

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.

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.