Page MenuHomeFreeBSD

Always null-terminate ccb_pathinq.(sim_vid|hba_vid|dev_name) (part 1)
ClosedPublic

Authored by asomers on Jan 4 2017, 12:10 AM.
Tags
None
Referenced Files
F131702230: D9037.diff
Fri, Oct 10, 11:39 AM
Unknown Object (File)
Fri, Oct 3, 7:19 PM
Unknown Object (File)
Fri, Oct 3, 11:06 AM
Unknown Object (File)
Thu, Oct 2, 3:19 PM
Unknown Object (File)
Thu, Oct 2, 3:10 PM
Unknown Object (File)
Sep 2 2025, 3:56 PM
Unknown Object (File)
Aug 6 2025, 3:13 AM
Unknown Object (File)
Aug 4 2025, 12:04 AM
Subscribers

Details

Summary

The sim_vid, hba_vid, and dev_name fields of struct ccb_pathinq are
fixed-length strings. AFAICT the only place they're read is in
sbin/camcontrol/camcontrol.c, which assumes they'll be null-terminated.
However, the kernel doesn't null-terminate them. A bunch of copy-pasted code
uses strncpy to write them, and doesn't guarantee null-termination. For at
least 4 drivers (mpr, mps, ciss, and hyperv), the hba_vid field actually
overflows. You can see the result by doing "camcontrol negotiate da0 -v".

This change null-terminates those fields everywhere they're set in the
kernel. It also shortens a few strings to ensure they'll fit within the
16-character field.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 6554
Build 6776: arc lint + arc unit

Event Timeline

asomers retitled this revision from to Always null-terminate ccb_pathinq.(sim_vid|hba_vid|dev_name) (part 1).
asomers updated this object.
asomers edited the test plan for this revision. (Show Details)
asomers added reviewers: ken, slm.
imp added a reviewer: imp.

This change is largely a no-op in terms of effect.
However, it eliminates dozens of false-positive uses of strncpy which people get wrong all the time and ensure that if cam_sim_name() is super-duper long we do something sensible (or at least fail-safe better than today's missing NUL). It's also a tiny performance win, but I doubt anybody can measure the effects of saving a few dozen cycles on boot.

This revision is now accepted and ready to land.Jan 4 2017, 2:17 AM