> -----Original Message-----
> From: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx>
> Sent: Monday, March 9, 2020 6:14 PM
> To: Changming Liu <liu.changm@xxxxxxxxxxxxxxxx>
> Cc: linux-media@xxxxxxxxxxxxxxx
> Subject: Re: [bug-report] drivers/media/platform/am437x/: illegal value of
> enum in vpfe_ccdc_set_params
>
> 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 ?
>
Thank you very much for the recognition! This means a lot to me.
I've emailed you a patch which can fix this issue via my gmail account, feel free
to see it as an advice and modify it as you wish to.
All the best,
-- Changming Liu
> Cheers,
> --Prabhakar
>
> > Looking forward to your response!
> >
> > Best regards,
> > Changming Liu
