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

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

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