Bounded flexible arrays: tag their element count

[seanm]

I took a stab at preparing for gcc/clang adding support for bounds checking of flexible arrays, see:

https://people.kernel.org/kees/bounded-flexible-arrays-in-c

And I think I found a small buffer overread in the process...

[mcuee]

Strange that umockdev_test failed in the CI for Linux.

[tormodvolden]

Can we isolate the bug fix for 1.0.27 and take the rest afterwards?

[seanm]

Can we isolate the bug fix...

First I need someone to review the WIP and tell me if I'm even correct or not. :)

1. have I correctly understood the size of these arrays?
2. is there indeed a buffer overrun as I suspect?

...and take the rest afterwards

Sure, I can split them.

[tormodvolden]

OK, so what you are saying is that if the device erroneously returns a string descriptor with an odd bLength, we would be reading a byte too much from the descriptor buffer, and if bLength=255 also beyond the buffer?

We are already issuing a warning on odd bLength, maybe we should just round bLength down to nearest even number before decoding the 16-bit characters? Well, maybe that's what your si_max is doing? :) Do we need all the size_of's? Is it even relevant what the storage size is, if we index them properly?

si and di are definitely source respectively destination index. di was used when reading from the source buffer because the latter is an array of 16-bit characters, as noted in a comment since it is a bit counter-intuitive. I guess in your version we wouldn't need both si and di.

[seanm]

OK, so what you are saying is that if the device erroneously returns a string descriptor with an odd bLength, we would be reading a byte too much from the descriptor buffer, and if bLength=255 also beyond the buffer?

Yes. It came about trying to figure out the count of usbi_string_descriptor.wData. Also, you can substitute 'maliciously' for 'erroneously'. :) In case it wasn't clear, the asserts in my WIP are triggered.

We are already issuing a warning on odd bLength

Correct. But it's just a warning and doesn't change the buffer read size.

maybe we should just round bLength down to nearest even number before decoding the 16-bit characters? Well, maybe that's what your si_max is doing? :)

Exactly.

Do we need all the size_of's? Is it even relevant what the storage size is, if we index them properly?

You mean int si_max = (str.desc.bLength - sizeof(uint8_t) - sizeof(uint8_t)) / sizeof(uint16_t);? Well, we could write int si_max = (str.desc.bLength - 2) / 2; if you prefer, but I thought the former was more explanatory, as it corresponds to the fields of the sturcture:

struct usbi_string_descriptor {
	uint8_t  bLength;
	uint8_t  bDescriptorType;
	uint16_t wData[LIBUSB_FLEXIBLE_ARRAY] LIBUSB_COUNTED_BY((bLength - 2) / 2);
} LIBUSB_PACKED;

Since compilers don't support it yet, I'm not sure how complicated the expressions within the 'counted_by' attribute can be...

si and di are definitely source respectively destination index.

Thanks for confirming.

di was used when reading from the source buffer because the latter is an array of 16-bit characters, as noted in a comment since it is a bit counter-intuitive.

Yeah. The source and destination buffers need not be the same size though, so I think it's much safer that they be individually indexed.

[seanm]

So assuming my analysis is correct here... I'll split this PR into 2 now...

[seanm]

...and take the rest afterwards

Sure, I can split them.

Done: #1432

[tormodvolden]

I will pick the LIBUSB_FLEXIBLE_ARRAY rename commit.

[seanm]

Looks like clang master just got counted_by merged, so we'll soon be able to actually try this...

llvm/llvm-project@164f85db876e

[seanm]

Looks like clang master just got counted_by merged, so we'll soon be able to actually try this...

So I've tried it with clang, and it works except it doesn't allow even simple expressions, so things like LIBUSB_COUNTED_BY(bLength - 3) alas don't work...