> 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
>
>
