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!
Looking forward to your response!
Best regards,
Changming Liu
