Re: [PATCH 2/4] media: venus: hfi_parser: avoid OOB access beyond payload word count

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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