kldxref: Avoid buffer overflows in parse_pnp_list
ClosedPublic
Actions

Authored by jrtc27 on Oct 2 2020, 4:02 AM.

Details

Reviewers

brooks
emaste
imp
jhb
kib

Commits

rS366736: kldxref: Avoid buffer overflows in parse_pnp_list

Summary

We convert a string like "W32:vendor/device" into "I:vendor;I:device",
where the output is longer than the input, but only allocate space equal
to the length of the input, leading to a buffer overflow.

Instead use open_memstream so we get a safe dynamically-grown buffer.

Found by: CHERI
Obtained from: CheriBSD

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 34137
Build 31297: arc lint + arc unit

Event Timeline

jrtc27 requested review of this revision.Oct 2 2020, 4:02 AM

jrtc27 created this revision.

Harbormaster completed remote builds in B33942: Diff 77766.Oct 2 2020, 4:02 AM

It would be better to just ignore this entry on errors... However, we don't do that for others, so that would be a stretch goal.

I'd originally thought this was safe since all strings were shorter, but this one clearly isn't... So I like your solution better.

usr.sbin/kldxref/kldxref.c
374	It would be better to just ignore this module in this case.

This revision is now accepted and ready to land.Oct 2 2020, 4:10 AM

kib added inline comments.Oct 2 2020, 10:10 AM

usr.sbin/kldxref/kldxref.c
374	I think a vitally important data to print for user is the kld name that is causing troubles. At least, the file can be removed and kldxref re-run.

jrtc27 added inline comments.Oct 2 2020, 4:14 PM

usr.sbin/kldxref/kldxref.c
374	I think a vitally important data to print for user is the kld name that is causing troubles. At least, the file can be removed and kldxref re-run. The same is true for all the other errors in this function and others like `record_buf` and `record_string`, as well as everything that calls `check`. Ideally the error reporting would be overhauled, but the file name is not currently available here so that change would be more invasive (and would only fix one of the many problems). Plus if you get one of these cryptic errors you can run it with `-v` to see what file it choked on.

jrtc27 added inline comments.Oct 2 2020, 4:16 PM

usr.sbin/kldxref/kldxref.c
374	Well, `check` itself is a bit dodgy (a hidden `break` on error) but not inherently problematic, except that parse_entry's return value is never consulted. Lots of issues here if you want good error reporting.