* [bug report] Revert "media: staging: atomisp: Remove driver"
@ 2021-03-12 6:43 Dan Carpenter
2021-03-12 7:24 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2021-03-12 6:43 UTC (permalink / raw)
To: mchehab+huawei; +Cc: linux-media
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().
262 asd->params.dvs_6axis = (struct ia_css_dvs_6axis_config *)
263 param->params.dvs_6axis;
264
265 /*
266 * WORKAROUND:
267 * Because the camera halv3 can't ensure to set zoom
268 * region to per_frame setting and global setting at
269 * same time and only set zoom region to pre_frame
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bug report] Revert "media: staging: atomisp: Remove driver"
2021-03-12 6:43 [bug report] Revert "media: staging: atomisp: Remove driver" Dan Carpenter
@ 2021-03-12 7:24 ` Mauro Carvalho Chehab
2021-03-12 10:08 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2021-03-12 7:24 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media, Sakari Ailus
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 ¶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.
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;
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [bug report] Revert "media: staging: atomisp: Remove driver"
2021-03-12 7:24 ` Mauro Carvalho Chehab
@ 2021-03-12 10:08 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2021-03-12 10:08 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media, Sakari Ailus
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 ¶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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [bug report] Revert "media: staging: atomisp: Remove driver"
@ 2020-06-26 10:42 Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-06-26 10:42 UTC (permalink / raw)
To: mchehab+huawei; +Cc: Sakari Ailus, linux-media
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_acc.c:506 atomisp_acc_load_extensions()
warn: iterator used outside loop: 'acc_fw'
drivers/staging/media/atomisp/pci/atomisp_acc.c
446 int atomisp_acc_load_extensions(struct atomisp_sub_device *asd)
447 {
448 struct atomisp_acc_fw *acc_fw;
449 bool ext_loaded = false;
450 bool continuous = asd->continuous_mode->val &&
451 asd->run_mode->val == ATOMISP_RUN_MODE_PREVIEW;
452 int ret = 0, i = -1;
453 struct atomisp_device *isp = asd->isp;
454
455 if (asd->acc.pipeline || asd->acc.extension_mode)
456 return -EBUSY;
457
458 /* Invalidate caches. FIXME: should flush only necessary buffers */
459 wbinvd();
460
461 list_for_each_entry(acc_fw, &asd->acc.fw, list) {
462 if (acc_fw->type != ATOMISP_ACC_FW_LOAD_TYPE_OUTPUT &&
463 acc_fw->type != ATOMISP_ACC_FW_LOAD_TYPE_VIEWFINDER)
464 continue;
465
466 for (i = 0; i < ARRAY_SIZE(acc_flag_to_pipe); i++) {
467 /* QoS (ACC pipe) acceleration stages are currently
468 * allowed only in continuous mode. Skip them for
469 * all other modes. */
470 if (!continuous &&
471 acc_flag_to_pipe[i].flag ==
472 ATOMISP_ACC_FW_LOAD_FL_ACC)
473 continue;
474
475 if (acc_fw->flags & acc_flag_to_pipe[i].flag) {
476 ret = atomisp_css_load_acc_extension(asd,
477 acc_fw->fw,
478 acc_flag_to_pipe[i].pipe_id,
479 acc_fw->type);
480 if (ret)
481 goto error;
The little loop is intended to clean up a partial iteration from this
goto.
482
483 ext_loaded = true;
484 }
485 }
486
487 ret = atomisp_css_set_acc_parameters(acc_fw);
488 if (ret < 0)
489 goto error;
And this one.
490 }
491
492 if (!ext_loaded)
493 return ret;
494
495 ret = atomisp_css_update_stream(asd);
496 if (ret) {
497 dev_err(isp->dev, "%s: update stream failed.\n", __func__);
498 goto error;
But if we hit this goto then "i" is non-zero and "acc_fw" is a bogus
pointer.
499 }
500
501 asd->acc.extension_mode = true;
502 return 0;
503
504 error:
505 while (--i >= 0) {
506 if (acc_fw->flags & acc_flag_to_pipe[i].flag) {
^^^^^^^^^^^^^
Don't we need to check:
if (!continuous &&
acc_flag_to_pipe[i].flag == ATOMISP_ACC_FW_LOAD_FL_ACC)
?
507 atomisp_css_unload_acc_extension(asd, acc_fw->fw,
508 acc_flag_to_pipe[i].pipe_id);
509 }
510 }
511
512 list_for_each_entry_continue_reverse(acc_fw, &asd->acc.fw, list) {
513 if (acc_fw->type != ATOMISP_ACC_FW_LOAD_TYPE_OUTPUT &&
514 acc_fw->type != ATOMISP_ACC_FW_LOAD_TYPE_VIEWFINDER)
515 continue;
516
517 for (i = ARRAY_SIZE(acc_flag_to_pipe) - 1; i >= 0; i--) {
518 if (!continuous &&
519 acc_flag_to_pipe[i].flag ==
520 ATOMISP_ACC_FW_LOAD_FL_ACC)
521 continue;
522 if (acc_fw->flags & acc_flag_to_pipe[i].flag) {
523 atomisp_css_unload_acc_extension(asd,
524 acc_fw->fw,
525 acc_flag_to_pipe[i].pipe_id);
526 }
527 }
528 }
529 return ret;
530 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* [bug report] Revert "media: staging: atomisp: Remove driver"
@ 2020-05-29 10:41 Dan Carpenter
2020-05-29 15:36 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2020-05-29 10:41 UTC (permalink / raw)
To: mchehab+huawei; +Cc: Sakari Ailus, linux-media
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_acc.c:207 atomisp_acc_load_to_pipe()
warn: pointer comes from user 'acc_fw->fw->blob.code'
drivers/staging/media/atomisp/pci/atomisp_acc.c
168
169 acc_fw = acc_alloc_fw(user_fw->size);
170 if (!acc_fw)
171 return -ENOMEM;
172
173 if (copy_from_user(acc_fw->fw, user_fw->data, user_fw->size)) {
^^^^^^^^^^
The acc_fw->fw->blob.code pointer isn't annotated as __user data.
Eventually it gets passed as "data" to int hmm_store() and treated as
a kernel pointer.
Presumably only privileged users can load new firmware so this isn't
a serious security bug...
174 acc_free_fw(acc_fw);
175 return -EFAULT;
176 }
177
178 handle = ida_alloc(&asd->acc.ida, GFP_KERNEL);
179 if (handle < 0) {
180 acc_free_fw(acc_fw);
181 return -ENOSPC;
182 }
183
184 user_fw->fw_handle = handle;
185 acc_fw->handle = handle;
186 acc_fw->flags = user_fw->flags;
187 acc_fw->type = user_fw->type;
188 acc_fw->fw->handle = handle;
189
190 /*
191 * correct isp firmware type in order ISP firmware can be appended
192 * to correct pipe properly
193 */
194 if (acc_fw->fw->type == ia_css_isp_firmware) {
195 static const int type_to_css[] = {
196 [ATOMISP_ACC_FW_LOAD_TYPE_OUTPUT] =
197 IA_CSS_ACC_OUTPUT,
198 [ATOMISP_ACC_FW_LOAD_TYPE_VIEWFINDER] =
199 IA_CSS_ACC_VIEWFINDER,
200 [ATOMISP_ACC_FW_LOAD_TYPE_STANDALONE] =
201 IA_CSS_ACC_STANDALONE,
202 };
203 acc_fw->fw->info.isp.type = type_to_css[acc_fw->type];
204 }
205
206 list_add_tail(&acc_fw->list, &asd->acc.fw);
207 return 0;
208 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bug report] Revert "media: staging: atomisp: Remove driver"
2020-05-29 10:41 Dan Carpenter
@ 2020-05-29 15:36 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2020-05-29 15:36 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Sakari Ailus, linux-media
Em Fri, 29 May 2020 13:41:07 +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_acc.c:207 atomisp_acc_load_to_pipe()
> warn: pointer comes from user 'acc_fw->fw->blob.code'
>
> drivers/staging/media/atomisp/pci/atomisp_acc.c
> 168
> 169 acc_fw = acc_alloc_fw(user_fw->size);
> 170 if (!acc_fw)
> 171 return -ENOMEM;
> 172
> 173 if (copy_from_user(acc_fw->fw, user_fw->data, user_fw->size)) {
> ^^^^^^^^^^
> The acc_fw->fw->blob.code pointer isn't annotated as __user data.
> Eventually it gets passed as "data" to int hmm_store() and treated as
> a kernel pointer.
>
> Presumably only privileged users can load new firmware so this isn't
> a serious security bug...
Yeah, the firmware file is received only at the device probe's time
(or at open).
On a side note, after looking on some things today, I'm not even sure if the
code under atomisp_acc is ever called. The firmware file is actually a
container with several binaries of different types: "normal" files,
and 3 types of "accel" files (used by this _acc code). At least at the
two firmware files I'm using on my tests, the only binaries available
are from the "normal" type.
In any case, except if someone write it first, I'll try to write a
patch for it (as the upcoming merge window would permit).
> 174 acc_free_fw(acc_fw);
> 175 return -EFAULT;
> 176 }
> 177
> 178 handle = ida_alloc(&asd->acc.ida, GFP_KERNEL);
> 179 if (handle < 0) {
> 180 acc_free_fw(acc_fw);
> 181 return -ENOSPC;
> 182 }
> 183
> 184 user_fw->fw_handle = handle;
> 185 acc_fw->handle = handle;
> 186 acc_fw->flags = user_fw->flags;
> 187 acc_fw->type = user_fw->type;
> 188 acc_fw->fw->handle = handle;
> 189
> 190 /*
> 191 * correct isp firmware type in order ISP firmware can be appended
> 192 * to correct pipe properly
> 193 */
> 194 if (acc_fw->fw->type == ia_css_isp_firmware) {
> 195 static const int type_to_css[] = {
> 196 [ATOMISP_ACC_FW_LOAD_TYPE_OUTPUT] =
> 197 IA_CSS_ACC_OUTPUT,
> 198 [ATOMISP_ACC_FW_LOAD_TYPE_VIEWFINDER] =
> 199 IA_CSS_ACC_VIEWFINDER,
> 200 [ATOMISP_ACC_FW_LOAD_TYPE_STANDALONE] =
> 201 IA_CSS_ACC_STANDALONE,
> 202 };
> 203 acc_fw->fw->info.isp.type = type_to_css[acc_fw->type];
> 204 }
> 205
> 206 list_add_tail(&acc_fw->list, &asd->acc.fw);
> 207 return 0;
> 208 }
>
> regards,
> dan carpenter
Thanks,
Mauro
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-12 10:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-12 6:43 [bug report] Revert "media: staging: atomisp: Remove driver" Dan Carpenter
2021-03-12 7:24 ` Mauro Carvalho Chehab
2021-03-12 10:08 ` Dan Carpenter
-- strict thread matches above, loose matches on Subject: below --
2020-06-26 10:42 Dan Carpenter
2020-05-29 10:41 Dan Carpenter
2020-05-29 15:36 ` Mauro Carvalho Chehab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).