[PATCH v5 15/16] media: platform: Add mali-c55 parameters video node

[Dan Scally]

Hi Sakari

On 14/06/2024 22:11, Sakari Ailus wrote:
> Hi Dan,
>
> On Fri, Jun 14, 2024 at 09:15:07PM +0100, Dan Scally wrote:
>>>> +void mali_c55_params_write_config(struct mali_c55 *mali_c55)
>>>> +{
>>>> +	struct mali_c55_params *params = &mali_c55->params;
>>>> +	enum vb2_buffer_state state = VB2_BUF_STATE_DONE;
>>>> +	struct mali_c55_params_buffer *config;
>>>> +	struct mali_c55_buffer *buf;
>>>> +	size_t block_offset = 0;
>>>> +
>>>> +	spin_lock(&params->buffers.lock);
>>>> +
>>>> +	buf = list_first_entry_or_null(&params->buffers.queue,
>>>> +				       struct mali_c55_buffer, queue);
>>>> +	if (buf)
>>>> +		list_del(&buf->queue);
>>>> +	spin_unlock(&params->buffers.lock);
>>>> +
>>>> +	if (!buf)
>>>> +		return;
>>>> +
>>>> +	buf->vb.sequence = mali_c55->isp.frame_sequence;
>>>> +	config = vb2_plane_vaddr(&buf->vb.vb2_buf, 0);
>>>> +
>>>> +	if (config->total_size > MALI_C55_PARAMS_MAX_SIZE) {
>>>> +		dev_dbg(mali_c55->dev, "Invalid parameters buffer size %lu\n",
>>>> +			config->total_size);
>>>> +		state = VB2_BUF_STATE_ERROR;
>>>> +		goto err_buffer_done;
>>>> +	}
>>>> +
>>>> +	/* Walk the list of parameter blocks and process them. */
>>>> +	while (block_offset < config->total_size) {
>>>> +		const struct mali_c55_block_handler *block_handler;
>>>> +		struct mali_c55_params_block_header *block;
>>>> +
>>>> +		block = (struct mali_c55_params_block_header *)
>>>> +			 &config->data[block_offset];
>>> How do you ensure config->data does hold a full struct
>>> mali_c33_params_block_header at block_offset (i.e. that the struct does not
>>> exceed the memory available for config->data)?
>>
>> We don't currently...the data buffer is sized specifically to be large
>> enough to accept a single instance of each possible struct that could be
>> included, we could keep track of the blocks that we have seen already and
>> ensure that none are seen twice...and that should guarantee that the
>> remaining space is sufficient to hold whatever the last block is. Does that
>> sound ok?
> Ḯ'd add an explicit check here.


How would you do the check, sorry?

> It's more simple way to ensure memory
> safety here: relying on a complex machinery that can't be trivially
> validated does risk having grave bugs, not only now but later on as well as
> modifications to the code are done.
>
>>>> +
>>>> +		if (block->type >= MALI_C55_PARAM_BLOCK_SENTINEL) {
>>>> +			dev_dbg(mali_c55->dev, "Invalid parameters block type\n");
>>>> +			state = VB2_BUF_STATE_ERROR;
>>>> +			goto err_buffer_done;
>>>> +		}
>>>> +
>>>> +		block_handler = &mali_c55_block_handlers[block->type];
>>>> +		if (block->size != block_handler->size) {
>>> How do you ensure config->data has room for the block?
>> I think through the same proposal as above.
> Similarly here. You already even have the size of the blocks available
> here.
>