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

[Dan Scally <dan.scally@xxxxxxxxxxxxxxxx>]

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.