Re: [bug report] Revert "media: staging: atomisp: Remove driver"

[Dan Carpenter <dan.carpenter@oracle.com>]

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@oracle.com> 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                                                        &param->params);
> >    249                          atomisp_apply_css_parameters(asd, &param->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                                          &param->params.dz_config, css_pipe_id);
> >    255                                  if (!err)
> >    256                                          asd->params.config.dz_config = &param->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