Page MenuHomeFreeBSD

md: Fix a read-after-free in BIO_GETATTR handling
ClosedPublic

Authored by markj on Dec 22 2020, 4:08 PM.

Details

Reviewers
kib
brooks
Summary

g_handleattr_int() consumes the bio if the attribute matches, so when we
check bp->bio_cmd bp may have been freed. For reference, this came in
with r341275 and r341765.

Move GETATTR handling to a separate function to avoid the problem. We
do not need to set bio_completed for such bios, g_handleattr_int() will
handle it.

I also removed the setting of bio_resid before the
devstat_end_transaction_bio() call. All of the md(4) bio handlers set
bio_resid already. I do not understand why we unconditionally set
bio_completed, this seems wrong if there was an I/O error.

Test Plan

KASAN found the bug when running the regression test suite.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 35618
Build 32517: arc lint + arc unit

Event Timeline

markj requested review of this revision.Dec 22 2020, 4:08 PM
markj added reviewers: kib, brooks.

bio_resid is typically recalculated from bio_completed in the geom code. I do not remember most of the details already.

md(4) seems to maintain bio_resid, in fact it is simpler for the code, because it typically pass uio which also maintains uio_resid. Perhaps bio_completed should be set to residual of bio_resid in md(4).

sys/dev/md/md.c
1190

Add != 0, to fix style and be more consistent.

This revision is now accepted and ready to land.Dec 22 2020, 4:54 PM
In D27724#619710, @kib wrote:

bio_resid is typically recalculated from bio_completed in the geom code. I do not remember most of the details already.

Right. bio_bcount and bio_resid are derived from bio_completed.

md(4) seems to maintain bio_resid, in fact it is simpler for the code, because it typically pass uio which also maintains uio_resid. Perhaps bio_completed should be set to residual of bio_resid in md(4).

Indeed, I think bio_completed should be derived from bio_resid. I will handle it in a separate change.