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

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

[Dan Carpenter]

Hello Ming Qian,

The patch d82977796c48: "media: amphion: implement windsor encoder
rpc interface" from Feb 24, 2022, leads to the following Smatch
static checker warning:

	drivers/media/platform/amphion/vpu_windsor.c:823 vpu_windsor_config_memory_resource()
	error: buffer overflow 'pool->enc_frames' 6 <= 7

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.

    824                 break;
    825         case MEM_RES_REF:
    826                 res = &pool->ref_frames[index];
    827                 break;
    828         case MEM_RES_ACT:
    829                 res = &pool->act_frame;
    830                 break;
    831         default:
    832                 return -EINVAL;
    833         }
    834 
    835         res->phys = buf->phys;
    836         res->virt = buf->phys - shared->boot_addr;
    837         res->size = buf->length;
    838 
    839         return 0;
    840 }

regards,
dan carpenter

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

[Ming Qian]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Thursday, March 10, 2022 2:57 PM
> To: Ming Qian <ming.qian@nxp.com>
> Cc: linux-media@vger.kernel.org
> Subject: [EXT] [bug report] media: amphion: implement windsor encoder rpc
> interface
> 
> Caution: EXT Email
> 
> Hello Ming Qian,
> 
> The patch d82977796c48: "media: amphion: implement windsor encoder rpc
> interface" from Feb 24, 2022, leads to the following Smatch static checker
> warning:
> 
>         drivers/media/platform/amphion/vpu_windsor.c:823
> vpu_windsor_config_memory_resource()
>         error: buffer overflow 'pool->enc_frames' 6 <= 7
> 
> 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).
    Maybe I should add a check for the index like:

	If (index >= ARRAY_SIZE(pool->enc_frames))
		return -EINVAL;

> 
>     824                 break;
>     825         case MEM_RES_REF:
>     826                 res = &pool->ref_frames[index];
>     827                 break;
>     828         case MEM_RES_ACT:
>     829                 res = &pool->act_frame;
>     830                 break;
>     831         default:
>     832                 return -EINVAL;
>     833         }
>     834
>     835         res->phys = buf->phys;
>     836         res->virt = buf->phys - shared->boot_addr;
>     837         res->size = buf->length;
>     838
>     839         return 0;
>     840 }
> 
> regards,
> dan carpenter

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

[Dan Carpenter]

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

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

[Ming Qian]

> 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()?
> 

Hi Dan,
    Yes, you're right, I have also noticed that.
    I will make a patch that add the checking code.
    Here the array size is defined in the firmware, so I think I should not change them.

    Thanks for your review.

Ming

> > >
> > >     824                 break;
> > >     825         case MEM_RES_REF:
> > >     826                 res = &pool->ref_frames[index];
> 
> And here as well.
> 

Yes, I'll check it too.

> regards,
> dan carpenter
> 
>