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(¶ms->buffers.lock);
> > > +
> > > + buf = list_first_entry_or_null(¶ms->buffers.queue,
> > > + struct mali_c55_buffer, queue);
> > > + if (buf)
> > > + list_del(&buf->queue);
> > > + spin_unlock(¶ms->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. 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.
--
Kind regards,
Sakari Ailus
