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
