Page MenuHomeFreeBSD

Fix multiple Coverity CWE-119 (Out-of-bounds access) errors in userland CAM code
ClosedPublic

Authored by truckman on May 23 2016, 2:30 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 21, 12:57 AM
Unknown Object (File)
Tue, Mar 12, 2:13 AM
Unknown Object (File)
Tue, Mar 12, 2:13 AM
Unknown Object (File)
Tue, Mar 12, 2:12 AM
Unknown Object (File)
Fri, Mar 8, 4:27 AM
Unknown Object (File)
Tue, Feb 27, 8:01 PM
Unknown Object (File)
Feb 24 2024, 7:12 PM
Unknown Object (File)
Feb 22 2024, 8:11 PM
Subscribers

Details

Summary

The currently used idiom for clearing the part of a ccb after its
headers generates one or two Coverity errors for each instance. All
instances generate an Out-of-bounds access (ARRAY_VS_SINGLETON)
error because of the treatment of the header as a two element array.
Some instances also alsp generate Out-of-bounds access (OVERRUN)
errors, probably because the space being cleared is larger than the
header struct.

In addition, this idiom is difficult for humans to understand and it
is error prone. The user has to chose the proper struct ccb_* type
(which does not appear in the surrounding code) for the sizeof() in
the length calculation. I found several instances where the length
was incorrect, which could cause either an actual out of bounds
write, or incompletely clear the ccb. These are noted inline below.
I think most of the problems flagged by Coverity are false
positives, but it is difficult to tell without studying each in
detail.

A better way is to write the code to clear the ccb itself starting
at sizeof(ccb_hdr) bytes from the start of the ccb, and calculate
the length based on the name of the union ccb member, which is used
elsewhere in the surrounding code. Furthermore, this can all be
wrapped in a convenient macro to avoid repetition of a the same
boiler-plate code in many places.

Test Plan

make universe

disassemble and compare the before and after .o files for the
modified sources

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

truckman retitled this revision from to Fix multiple Coverity CWE-119 (Out-of-bounds access) errors in userland CAM code.
truckman updated this object.
truckman edited the test plan for this revision. (Show Details)
lib/libcam/camlib.c
622 ↗(On Diff #16698)

sizeof(struct ccb_hdr) was not subtracted from the length here. That could cause bzero() to write past the end of the ccb.

sbin/camcontrol/attrib.c
141 ↗(On Diff #16698)

I believe the length calculation should have used sizeof(struct ccb_scsiio) instead of sizeof(union_ccb).

sbin/camcontrol/camcontrol.c
4932 ↗(On Diff #16698)

sizeof(struct ccb_getdev) should have been used here instead of sizeof(struct ccb_pathinq). This error could result in incomplete initialization if sizeof(struct ccb_pathinq) is smaller.

7031 ↗(On Diff #16698)

It would be more efficient to use sizeof(struct ccb_smpio) here instead of sizeof(union ccb).

7225 ↗(On Diff #16698)

Same as previous

7288 ↗(On Diff #16698)

Same as previous.

7371 ↗(On Diff #16698)

Same as previous.

7629 ↗(On Diff #16698)

Same as previous

7724 ↗(On Diff #16698)

It would be more efficient (but more typing) to use sizeof(struct ccb_dev_advinfo) here instead of sizeof(union ccb).

8010 ↗(On Diff #16698)

It would be more efficient to use sizeof(struct ccb_smpio) here.

8115 ↗(On Diff #16698)

Same as previous.

sbin/camcontrol/persist.c
454 ↗(On Diff #16698)

It would be more efficent to use sizeof(struct scsiio) here.

usr.sbin/camdd/camdd.c
1391 ↗(On Diff #16698)

This is incorrectly using sizeof(struct ccb_scsiio) instead of sizeof(struct ccb_pathinq).

Looks good to me but I would like to have someone from cam@ to approve this.

I just counted 'em up and found that this is responsible for 77 Coverity errors.

scottl edited edge metadata.
This revision is now accepted and ready to land.May 23 2016, 4:16 PM

Any thoughts about MFC to 10-STABLE?

Pro: diff reduction to avoid problems with other MFCs.

Con: possibility of introducing bugs

If I specify MFC after, I think 1 month would be appropriate.

ken edited edge metadata.

This looks good, thank you for tackling it, and thank you for doing it in a clean, reasonable way!

I think an MFC is fine, and 1 month should be plenty to hopefully catch any regressions.

lib/libcam/camlib.c
622 ↗(On Diff #16698)

In this case, struct ccb_trans_settings is likely small enough that it wouldn't cause an issue. But your point is valid nonetheless.

delphij edited edge metadata.
imp added a reviewer: imp.

I like the code cleanup, but foo[1] is a time-honored technique to get the address following foo. I'm surprised this would be flagged as an error.

Still, there were a few issues with cut and paste that the macro nicely fixes...

In D6496#137886, @imp wrote:

I like the code cleanup, but foo[1] is a time-honored technique to get the address following foo. I'm surprised this would be flagged as an error.

Coverity is interpreting the code very literally. It sees that we are writing to a non-existent array element and thinks that will cause data outside of the array to be corrupted. It also complains if the length passed to bzero() is larger than the length of the object. Changing the code to clear a portion of the containing struct should make it happy because the operation is contained entirely within that struct and won't corrupt anything adjacent to it in memory.

I fixed a simpler false positive of the latter type in r300002.

This revision was automatically updated to reflect the committed changes.