On Fri, Mar 12, 2021 at 08:24:33AM +0100, Mauro Carvalho Chehab wrote:
> Hi Dan,
>
> Em Fri, 12 Mar 2021 09:43:44 +0300
> Dan Carpenter <dan.carpenter@xxxxxxxxxx> escreveu:
>
> > Hello Mauro Carvalho Chehab,
> >
> > The patch ad85094b293e: "Revert "media: staging: atomisp: Remove
> > driver"" from Apr 19, 2020, leads to the following static checker
> > warning:
> >
> > drivers/staging/media/atomisp/pci/atomisp_fops.c:261 atomisp_q_video_buffers_to_css()
> > error: buffer overflow 'asd->stream_env[stream_id]->pipes' 6 <= 6
> >
> > drivers/staging/media/atomisp/pci/atomisp_fops.c
> > 234 list_del_init(&vb->queue);
> > 235 vb->state = VIDEOBUF_ACTIVE;
> > 236 spin_unlock_irqrestore(&pipe->irq_lock, irqflags);
> > 237
> > 238 /*
> > 239 * If there is a per_frame setting to apply on the buffer,
> > 240 * do it before buffer en-queueing.
> > 241 */
> > 242 vm_mem = vb->priv;
> > 243
> > 244 param = pipe->frame_params[vb->i];
> > 245 if (param) {
> > 246 atomisp_makeup_css_parameters(asd,
> > 247 &asd->params.css_param.update_flag,
> > 248 ¶m->params);
> > 249 atomisp_apply_css_parameters(asd, ¶m->params);
> > 250
> > 251 if (param->params.update_flag.dz_config &&
> > 252 asd->run_mode->val != ATOMISP_RUN_MODE_VIDEO) {
> > 253 err = atomisp_calculate_real_zoom_region(asd,
> > 254 ¶m->params.dz_config, css_pipe_id);
> > 255 if (!err)
> > 256 asd->params.config.dz_config = ¶m->params.dz_config;
> > 257 }
> > 258 atomisp_css_set_isp_config_applied_frame(asd,
> > 259 vm_mem->vaddr);
> > 260 atomisp_css_update_isp_params_on_pipe(asd,
> > 261 asd->stream_env[stream_id].pipes[css_pipe_id]);
> > ^^^^^^^^^^^
> > Can this be IA_CSS_PIPE_ID_NUM? It looks that way. The concern is
> > about the last caller in atomisp_qbuffers_to_css().
>
> Well, atomisp_q_video_buffers_to_css() should never receive
> IA_CSS_PIPE_ID_NUM in practice.
>
> See, the atomisp driver uses several different pipelines in order
> to capture images and do different types of image processing (like
> scaling, image improvements and format conversion). Those are
> dynamically set internally inside the driver's code, depending on
> the parameters set via different ioctls before starting to stream.
>
> On other words, calling the function with IA_CSS_PIPE_ID_NUM is
> invalid.
>
> So, I guess that the best fix would be to do something like the
> enclosed path.
>
drivers/staging/media/atomisp/pci/atomisp_fops.c
411 int atomisp_qbuffers_to_css(struct atomisp_sub_device *asd)
412 {
413 enum ia_css_buffer_type buf_type;
414 enum ia_css_pipe_id css_capture_pipe_id = IA_CSS_PIPE_ID_NUM;
415 enum ia_css_pipe_id css_preview_pipe_id = IA_CSS_PIPE_ID_NUM;
416 enum ia_css_pipe_id css_video_pipe_id = IA_CSS_PIPE_ID_NUM;
417 enum atomisp_input_stream_id input_stream_id;
418 struct atomisp_video_pipe *capture_pipe = NULL;
419 struct atomisp_video_pipe *vf_pipe = NULL;
420 struct atomisp_video_pipe *preview_pipe = NULL;
At the start of the atomisp_qbuffers_to_css() function we initialize
the pipe_id's to one element outside the array to silence a GCC warning
about unitialized variables. It would be less confusing to just
initialize it to zero.
regards,
dan carpenter
