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