On 12/11/2024 08:05, Vikash Garodia wrote:
You did not printed the last iteration without the proposed fix. In the last
iteration (Myword 1), it would access the data beyond allocated size of somebuf.
So we can see how the fix protects from OOB situation.
Right but the loop _can't_ be correct. What's the point in fixing an OOB
in a loop that doesn't work ?
This is the loop:
#define BUF_SIZE 0x20 // BUF_SIZE doesn't really matter
char somebuf[BUF_SIZE];
u32 *word = somebuf[0];
u32 words = ARRAY_SIZE(somebuf);
while (words > 1) {
data = word + 1; // this
word++; // and this
words--;
}
On the first loop
word = somebuf[0];
data = somebuf[3];
On the second loop
word = somebuf[3]; // the same value as *data in the previous loop
and that's just broken because on the second loop *word == *data in the
first loop !
That's what my program showed you
word 4 == 0x03020100 data=0x07060504
// word == data from previous loop
word 3 == 0x07060504 data=0x0b0a0908
// word == data from previous loop
word 2 == 0x0b0a0908 data=0x0f0e0d0c
The step size, the number of bytes this loop increments is fundamentally
wrong because
a) Its a fixed size [1]
b) *word in loop(n+1) == *data in loop(n)
Which cannot ever parse more than one data item - in effect never loop -
in one go.
For the functionality part, packet from firmware would come as <prop type>
followed by <payload for that prop> i.e
*word = HFI_PROPERTY_PARAM_CODEC_SUPPORTED
*data = payload --> hence here data is pointed to next u32 to point and parse
payload for HFI_PROPERTY_PARAM_CODEC_SUPPORTED.
likewise for other properties in the same packet
[1]
But we've established that word increments by one word.
We wouldn't fix this loop by just making it into
while (words > 1) {
data = word + 1;
word = data + 1;
words -= 2;
}
Because the consumers of the data have different step sizes, different
number of bytes they consume for the structs they cast.
=>
case HFI_PROPERTY_PARAM_CODEC_SUPPORTED:
parse_codecs(core, data);
// consumes sizeof(struct hfi_codec_supported)
struct hfi_codec_supported {
u32 dec_codecs;
u32 enc_codecs;
};
case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED:
parse_max_sessions(core, data);
// consumes sizeof(struct hfi_max_sessions_supported)
struct hfi_max_sessions_supported {
u32 max_sessions;
};
case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED:
parse_codecs_mask(&codecs, &domain, data);
// consumes sizeof(struct hfi_codec_mask_supported)
struct hfi_codec_mask_supported {
u32 codecs;
u32 video_domains;
};
case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED:
parse_raw_formats(core, codecs, domain, data);
// consumes sizeof(struct hfi_uncompressed_format_supported)
struct hfi_uncompressed_format_supported {
u32 buffer_type;
u32 format_entries;
struct hfi_uncompressed_plane_info plane_info;
};
case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED:
parse_caps(core, codecs, domain, data);
struct hfi_capabilities {
u32 num_capabilities;
struct hfi_capability data[];
};
where
hfi_platform.h:#define MAX_CAP_ENTRIES 32
I'll stop there.
This routine needs a rewrite.
---
bod
