Em Fri, 29 May 2020 13:41:07 +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_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
