Re: [EXT] [bug report] media: amphion: implement windsor encoder rpc interface

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

On Thu, Mar 10, 2022 at 07:34:56AM +0000, Ming Qian wrote:
> > drivers/media/platform/amphion/vpu_windsor.c
> >     807 int vpu_windsor_config_memory_resource(struct vpu_shared_addr
> > *shared,
> >     808                                        u32 instance,
> >     809                                        u32 type,
> >     810                                        u32 index,
> >     811                                        struct vpu_buffer *buf)
> >     812 {
> >     813         struct vpu_enc_mem_pool *pool;
> >     814         struct vpu_enc_memory_resource *res;
> >     815
> >     816         if (instance >= VID_API_NUM_STREAMS)
> >                                 ^^^^^^^^^^^^^^^^^^^ This is 8.
> > 
> >     817                 return -EINVAL;
> >     818
> >     819         pool = get_mem_pool(shared, instance);
> >     820
> >     821         switch (type) {
> >     822         case MEM_RES_ENC:
> > --> 823                 res = &pool->enc_frames[index];
> > 
> > This only has WINDSOR_MAX_SRC_FRAMES elements.
> 
> Hi Dan,
>     I don't get the point, the instance and index is different, and
> one vpu core can support 8 instances (VID_API_NUM_STREAMS),
> The enc_frame count of one instance won't exceed 6 (WINDSOR_MAX_SRC_FRAMES).

Hi Ming,

I appologize.  I mis-understood what Smatch was saying and mis-read the
code as well.  I got "instance" and "index" mixed up as you say.

However, when I re-reviewed the code now it looks like Smatch is correct
and there is a potential buffer overflow.  The "index" variable comes
from vpu_iface_unpack_msg_data() so I do not think it can be trusted.
We need to have an upper bounds.

There are 3 upper bounds checks in venc_request_mem_resource() but they
only check that "index" is in the 0-7 range.

	if (enc_frame_num > ARRAY_SIZE(venc->enc)) {
	if (ref_frame_num > ARRAY_SIZE(venc->ref)) {
	if (act_frame_num > ARRAY_SIZE(venc->act)) {

These ->enc, ->ref and ->act arrays all have 8 elements.

However, as noted by Smatch the pool->enc_frames[] array only has 6
elements and the pool->ref_frames[] array only has 3 elements.  So we
need to add bounds checking before both accesses.

>     Maybe I should add a check for the index like:
> 
> 	If (index >= ARRAY_SIZE(pool->enc_frames))
> 		return -EINVAL;
> 

Yes, please.  Or maybe we need to make the arrays larger to match the
arrays in venc_request_mem_resource()?

> > 
> >     824                 break;
> >     825         case MEM_RES_REF:
> >     826                 res = &pool->ref_frames[index];

And here as well.

regards,
dan carpenter