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

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

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.

Thanks,
Mauro

[PATCH] atomisp: don't let it go past pipes array

In practice, IA_CSS_PIPE_ID_NUM should never be used when
calling atomisp_q_video_buffers_to_css(), as the driver should
discover the right pipe before calling it.

Yet, if some pipe parsing issue happens, it could end using
it.

So, add a WARN_ON() to prevent such case.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c
index 453bb6913550..f1e6b2597853 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
@@ -221,6 +221,9 @@ int atomisp_q_video_buffers_to_css(struct atomisp_sub_device *asd,
 	unsigned long irqflags;
 	int err = 0;
 
+	if (WARN_ON(css_pipe_id >= IA_CSS_PIPE_ID_NUM))
+		return -EINVAL;
+
 	while (pipe->buffers_in_css < ATOMISP_CSS_Q_DEPTH) {
 		struct videobuf_buffer *vb;