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