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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Fri, Jun 14, 2024 at 10:49:37PM +0100, Daniel Scally wrote:
> On 14/06/2024 22:11, Sakari Ailus wrote:
> > 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?

You could simply change the while() loop to

	max_offset = config->total_size - sizeof(mali_c55_params_block_header);
	while (block_offset <= max_offset) {

That would ensure that you always have enough space left for a header.
Within the loop, you will need to check that block->size doesn't go past
the end of the remaining space. Please also check the code for integer
overflows.

> > 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.

-- 
Regards,

Laurent Pinchart