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
