* [bug report] media: amphion: implement windsor encoder rpc interface
@ 2022-03-10 6:56 Dan Carpenter
2022-03-10 7:34 ` [EXT] " Ming Qian
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2022-03-10 6:56 UTC (permalink / raw)
To: ming.qian; +Cc: linux-media
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [EXT] [bug report] media: amphion: implement windsor encoder rpc interface
2022-03-10 6:56 [bug report] media: amphion: implement windsor encoder rpc interface Dan Carpenter
@ 2022-03-10 7:34 ` Ming Qian
2022-03-10 11:45 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Ming Qian @ 2022-03-10 7:34 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media
> -----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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [EXT] [bug report] media: amphion: implement windsor encoder rpc interface
2022-03-10 7:34 ` [EXT] " Ming Qian
@ 2022-03-10 11:45 ` Dan Carpenter
2022-03-10 13:56 ` 回复: " Ming Qian
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2022-03-10 11:45 UTC (permalink / raw)
To: Ming Qian; +Cc: linux-media
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* 回复: [EXT] [bug report] media: amphion: implement windsor encoder rpc interface
2022-03-10 11:45 ` Dan Carpenter
@ 2022-03-10 13:56 ` Ming Qian
0 siblings, 0 replies; 4+ messages in thread
From: Ming Qian @ 2022-03-10 13:56 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media
> 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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-10 13:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-10 6:56 [bug report] media: amphion: implement windsor encoder rpc interface Dan Carpenter
2022-03-10 7:34 ` [EXT] " Ming Qian
2022-03-10 11:45 ` Dan Carpenter
2022-03-10 13:56 ` 回复: " Ming Qian
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).