Re: [PATCH v18 10/15] media: amphion: implement malone decoder rpc interface

[Dan Carpenter <dan.carpenter@oracle.com>]

On Thu, Feb 24, 2022 at 11:10:08AM +0800, Ming Qian wrote:
> +static int vpu_malone_add_padding_scode(struct vpu_buffer *stream_buffer,
> +					struct vpu_malone_str_buffer __iomem *str_buf,
> +					u32 pixelformat, u32 scode_type)
> +{
> +	u32 wptr;
> +	u32 size;
> +	u32 total_size = 0;
> +	const struct malone_padding_scode *ps;
> +	const u32 padding_size = 4096;
> +	int ret;
> +
> +	ps = get_padding_scode(scode_type, pixelformat);
> +	if (!ps)
> +		return -EINVAL;
> +
> +	wptr = readl(&str_buf->wptr);
> +	size = ALIGN(wptr, 4) - wptr;

The ALIGN() macro can wrap to zero if wptr is > UINT_MAX - 4.  This
would make size into a very high unsigned value.

> +	if (size)
> +		vpu_helper_memset_stream_buffer(stream_buffer, &wptr, 0, size);
> +	total_size += size;
> +
> +	size = sizeof(ps->data);
> +	ret = vpu_helper_copy_to_stream_buffer(stream_buffer, &wptr, size, (void *)ps->data);
> +	if (ret < size)

The problem here is that size is a u32 so ret is type promoted to a u32
and (u32)-EINVAL > size so the condition is impossible.

> +		return -EINVAL;
> +	total_size += size;
> +
> +	size = padding_size - sizeof(ps->data);
> +	vpu_helper_memset_stream_buffer(stream_buffer, &wptr, 0, size);
> +	total_size += size;
> +
> +	vpu_malone_update_wptr(str_buf, wptr);
> +	return total_size;

What was the point of making total_size a u32 if the function itself is
and int?

> +}

[ snip ]

> +static int vpu_malone_input_frame_data(struct vpu_malone_str_buffer __iomem *str_buf,
> +				       struct vpu_inst *inst, struct vb2_buffer *vb,
> +				       u32 disp_imm)
> +{
> +	struct malone_scode_t scode;
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	u32 wptr = readl(&str_buf->wptr);
> +	int size = 0;
> +	int ret = 0;
> +
> +	/*add scode: SCODE_SEQUENCE, SCODE_PICTURE, SCODE_SLICE*/
> +	scode.inst = inst;
> +	scode.vb = vb;
> +	scode.wptr = wptr;
> +	scode.need_data = 1;
> +	if (vbuf->sequence == 0 || vpu_vb_is_codecconfig(vbuf))
> +		ret = vpu_malone_insert_scode(&scode, SCODE_SEQUENCE);
> +
> +	if (ret < 0)
> +		return -ENOMEM;
> +	size += ret;
> +	wptr = scode.wptr;
> +	if (!scode.need_data) {
> +		vpu_malone_update_wptr(str_buf, wptr);
> +		return size;
> +	}
> +
> +	ret = vpu_malone_insert_scode(&scode, SCODE_PICTURE);
> +	if (ret < 0)
> +		return -ENOMEM;
> +	size += ret;
> +	wptr = scode.wptr;
> +
> +	ret = vpu_helper_copy_to_stream_buffer(&inst->stream_buffer,
> +					       &wptr,
> +					       vb2_get_plane_payload(vb, 0),
> +					       vb2_plane_vaddr(vb, 0));
> +	if (ret < vb2_get_plane_payload(vb, 0))

Here again, negative values of "ret" are type promoted to high unsigned
values so the condition is impossible.

> +		return -ENOMEM;
> +	size += ret;
> +
> +	vpu_malone_update_wptr(str_buf, wptr);
> +
> +	if (disp_imm && !vpu_vb_is_codecconfig(vbuf)) {
> +		ret = vpu_malone_add_scode(inst->core->iface,
> +					   inst->id,
> +					   &inst->stream_buffer,
> +					   inst->out_format.pixfmt,
> +					   SCODE_PADDING_BUFFLUSH);
> +		if (ret < 0)
> +			return ret;
> +		size += ret;
> +	}
> +
> +	return size;
> +}
> +
> +static int vpu_malone_input_stream_data(struct vpu_malone_str_buffer __iomem *str_buf,
> +					struct vpu_inst *inst, struct vb2_buffer *vb)
> +{
> +	u32 wptr = readl(&str_buf->wptr);
> +	int ret = 0;
> +
> +	ret = vpu_helper_copy_to_stream_buffer(&inst->stream_buffer,
> +					       &wptr,
> +					       vb2_get_plane_payload(vb, 0),
> +					       vb2_plane_vaddr(vb, 0));
> +	if (ret < vb2_get_plane_payload(vb, 0))

Same thing.  This condition is impossible.

> +		return -ENOMEM;
> +
> +	vpu_malone_update_wptr(str_buf, wptr);
> +
> +	return ret;
> +}

regards,
dan carpenter