kldxref: Avoid buffer overflows in parse_pnp_list

[jrtc27]

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

[imp]

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

383 ↗

(On Diff #77766)

It would be better to just ignore this module in this case.

[kib]

usr.sbin/kldxref/kldxref.c

383 ↗

(On Diff #77766)

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]

usr.sbin/kldxref/kldxref.c

383 ↗

(On Diff #77766)

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]

usr.sbin/kldxref/kldxref.c

383 ↗

(On Diff #77766)

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.

[jhb]

I would suggest using open_memstream() and let it auto-grow the buffer. You would only need a single error check at the point of fclose().

[jrtc27]

Use open_memstream instead of a fixed buffer size

[imp]

Looks good to me. Beats using sbuf since it is more portable

[jrtc27]

Avoid shadowing existing char type[8]

The code was correct but it's confusing.