Re: [PATCH v18 04/15] media: amphion: add vpu core driver

[Dan Carpenter]

On Thu, Feb 24, 2022 at 11:10:02AM +0800, Ming Qian wrote:
> +struct vpu_inst *vpu_core_find_instance(struct vpu_core *core, u32 index)
> +{
> +	struct vpu_inst *inst = NULL;
> +	struct vpu_inst *tmp;
> +
> +	mutex_lock(&core->lock);
> +	if (!test_bit(index, &core->instance_mask))

The "index" value comes from vpu_handle_msg() so I think it's untrusted
and this test_bit() can read way out of bounds.  It needs to be:

	if (index < BITS_PER_LONG && !test_bit(index, &core->instance_mask))


> +		goto exit;
> +	list_for_each_entry(tmp, &core->instances, list) {
> +		if (tmp->id == index) {
> +			inst = vpu_inst_get(tmp);
> +			break;
> +		}
> +	}
> +exit:
> +	mutex_unlock(&core->lock);
> +
> +	return inst;
> +}

[ snip ]

> +static int vpu_rpc_send_cmd_buf(struct vpu_shared_addr *shared, struct vpu_rpc_event *cmd)
> +{
> +	struct vpu_rpc_buffer_desc *desc;
> +	u32 space = 0;
> +	u32 *data;
> +	u32 wptr;
> +	u32 i;
> +
> +	desc = shared->cmd_desc;
> +	space = vpu_rpc_check_buffer_space(desc, true);
> +	if (space < (((cmd->hdr.num + 1) << 2) + 16))

In the current code the math here cannot overflow.  But it seems like
we could easly add a check:

	if (cmd->hdr.num > 0xff)
		return -EINVAL;

> +		return -EINVAL;
> +	wptr = desc->wptr;
> +	data = (u32 *)(shared->cmd_mem_vir + desc->wptr - desc->start);
> +	*data = 0;
> +	*data |= ((cmd->hdr.index & 0xff) << 24);
> +	*data |= ((cmd->hdr.num & 0xff) << 16);
> +	*data |= (cmd->hdr.id & 0x3fff);
> +	wptr += 4;
> +	data++;
> +	if (wptr >= desc->end) {
> +		wptr = desc->start;
> +		data = shared->cmd_mem_vir;
> +	}
> +
> +	for (i = 0; i < cmd->hdr.num; i++) {
> +		*data = cmd->data[i];
> +		wptr += 4;
> +		data++;
> +		if (wptr >= desc->end) {
> +			wptr = desc->start;
> +			data = shared->cmd_mem_vir;
> +		}
> +	}
> +
> +	/*update wptr after data is written*/
> +	mb();
> +	desc->wptr = wptr;
> +
> +	return 0;
> +}
> +
> +static bool vpu_rpc_check_msg(struct vpu_shared_addr *shared)
> +{
> +	struct vpu_rpc_buffer_desc *desc;
> +	u32 space = 0;
> +	u32 msgword;
> +	u32 msgnum;
> +
> +	desc = shared->msg_desc;
> +	space = vpu_rpc_check_buffer_space(desc, 0);
> +	space = (space >> 2);
> +
> +	if (space) {

It would be nicer if this condition were:

	if (space >= sizeof(u32)) {

> +		msgword = *(u32 *)(shared->msg_mem_vir + desc->rptr - desc->start);
> +		msgnum = (msgword & 0xff0000) >> 16;
> +		if (msgnum <= space)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +

regards,
dan carpenter