Hi Changming
On Sun, Mar 8, 2020 at 3:32 AM Changming Liu
<liu.changm@xxxxxxxxxxxxxxxx> wrote:
>
> This email was sent due to the previous one was rejected because of it's in HTML form.
>
> From: Changming Liu
> Sent: Saturday, March 7, 2020 8:33 PM
> To: prabhakar.csengg@xxxxxxxxx
> Cc: linux-media@xxxxxxxxxxxxxxx; yaohway@xxxxxxxxx
> Subject: [bug-report] drivers/media/platform/am437x/: illegal value of enum in vpfe_ccdc_set_params
>
> Hi Lad,
> Greetings, I'm a first-year PhD student who is interested in the usage of UBSan in linux kernel. With some experiments, I've found that, a unsigned underflow might cause undesired behavior in
> drivers/media/platform/am437x/am437x-vpfe.c function vpfe_ccdc_set_params.
>
> More specifically, after the execution of
> x = copy_from_user(&raw_params, params, sizeof(raw_params));
> the raw_params are filled with data from user space.
>
> Then diving into function vpfe_ccdc_validate_param, when calling function ccdc_data_size_max_bit, at
> max_data = ccdc_data_size_max_bit(ccdcparam->data_sz);
> the enum member, named data_sz, of structure ccdcparam is compared with 7, otherwise data_sz is subtracted from 15, as in
> return sz == VPFE_CCDC_DATA_8BITS ? 7 : 15 - sz;
>
> The potential problem with this snippet of code is that, although in function ccdc_data_size_max_bit, ccdcparam->data_sz is treated as an enumeration with the range from 0 to 7 according to the definition of enum vpfe_ccdc_data_size, however it's essentially an unsigned 32 bit integer from user space. As a consequence, the return value of function ccdc_data_size_max_bit might be any value from 0 to 255 due to the unsigned underflow and truncation.
>
> It's worth noting that, although the usage of function of ccdc_gamma_width_max_bit has similar underflow problem, i.e. the value of ccdcparam->alaw.gamma_wd is also an unsigned 32 bit from user space, while itself is a enum type. However it's checked in
> if (ccdcparam->alaw.gamma_wd > VPFE_CCDC_GAMMA_BITS_09_0 ||
> max_gamma > max_data) {
> vpfe_dbg(1, vpfe, "Invalid data line select\n");
> return -EINVAL;
> }
> This if clause exclude all illegal values and keep the enum variable in range, I wonder if it's necessary to apply the similar check to ccdcparam->data_sz to keep the its value legal as well.
>
> Due to the lack of knowledge of the interaction between this module and the user space, I'm not able to assess if this is a security-related issue. Judging from the appearance, a malicious user can possibly manipulate the return value of ccdc_data_size_max_bit and make the check of "max_gamma > max_data" always pass. I'd be more than happy to hear your valuable opinions and provide more information if needed. If such a check is unnecessary, I would appreciate it if I can know why, this can help me understand linux a lot!
>
Totally agree (good catch!), vpfe_ccdc_validate_param() should be more
stringent on checking the user space params. Would you create a patch
fixing it ?
Cheers,
--Prabhakar
> Looking forward to your response!
>
> Best regards,
> Changming Liu
