[Coverity CID: 220438] Out-of-bounds access in subsys/bluetooth/audio/vocs_client.c

[zephyrbot]

Static code scan issues found in file:

https:/zephyrproject-rtos/zephyr/tree/169144afa1826511ee6ec3f53d590b2c0d39d3d4/subsys/bluetooth/audio/vocs_client.c

Category: Memory - corruptions
Function: vocs_discover_func
Component: Bluetooth
CID: 220438

Details:

https:/zephyrproject-rtos/zephyr/blob/169144afa1826511ee6ec3f53d590b2c0d39d3d4/subsys/bluetooth/audio/vocs_client.c

Please fix or provide comments in coverity using the link:

https://scan9.coverity.com/reports.htm#v29271/p12996.

Note: This issue was created automatically. Priority was set based on classification
of the file affected and the impact field in coverity. Assignees were set using the CODEOWNERS file.

[Thalley]

This error seem to occur to all occurrences of of bt_uuid_cmp for both VOCS (vocs.c) and VOCS client (vocs_client.c).

To me it seems more like an issue with bt_uuid_cmp than the VOCS implementations.

[Thalley]

This is similar to #33808

[joerchan]

@Thalley You linked this issue to itself, did you mean another one?

[Thalley]

@Thalley You linked this issue to itself, did you mean another one?

Yes, thanks! :)

[joerchan]

This error seem to occur to all occurrences of of bt_uuid_cmp for both VOCS (vocs.c) and VOCS client (vocs_client.c).

It does appear for almost all occurrences of bt_uuid_cmp, we have marked them all as false positives.
Not sure how to fix it, but coverity clearly sees that the UUID is of type 16, and then says there would be out-of-bounds access if you take the type 128 switch case.

If you know how to make coverity stop reporting these it would help a lot with reducing noise from these reports.

[Thalley]

This error seem to occur to all occurrences of of bt_uuid_cmp for both VOCS (vocs.c) and VOCS client (vocs_client.c).

It does appear for almost all occurrences of bt_uuid_cmp, we have marked them all as false positives.
Not sure how to fix it, but coverity clearly sees that the UUID is of type 16, and then says there would be out-of-bounds access if you take the type 128 switch case.

If you know how to make coverity stop reporting these it would help a lot with reducing noise from these reports.

Thanks, that was also my assumption of what has been done previously.

Unfortunately (as mentioned on Slack) it isn't possible to run coverity locally, so I can't easily test things. I'm not sure what really causes the "which accesses it at byte offset 16." issue though.

[dleach02]

guys, I think the fix for this is in bt_uuid_cmp() function. The first test is if the types don't align to just go do a uuid128_cmp() which generically copies the UUID from both parameters as if they are 128bit but the original second parameter of BT_UUID_VOCS_DESCRIPTION is a UUID16.

I think you want uuid128_cmp() to detect the size of the UUIDs being compared and copy the appropriate size then do the memcmp().

[dleach02]

Nevermind. This is a false positive. Coverity is picking up on the fact that the uuid_to_uuid128() switch statement could access past the size but it isn't able to tell that the parameter passed in would have a type of 16.

I'm not sure coverity is smart enough to know the type field that was applied. Instead, it is tracking the size of the src/dst.

[Thalley]

@dleach02 Yeah, thanks, we marked it as false positive, but I forgot to close the Github issues :)

[Thalley]

Marked as false positive.