Re: [PATCH 1/1] ipu3-imgu: Fix NULL pointer dereference in active selection access

[Maximilian Luz <luzmaximilian@gmail.com>]

Hi,

On 9/7/22 21:09, Laurent Pinchart wrote:
> Hi Max,
> 
> On Wed, Sep 07, 2022 at 03:44:44PM +0200, Maximilian Luz wrote:
>> On Thu, 25 Aug 2022 22:03:51 +0300, Sakari Ailus  wrote:
>>> What the IMGU driver did was that it first acquired the pointers to active
>>> and try V4L2 subdev state, and only then figured out which one to use.
>>>
>>> The problem with that approach and a later patch (see Fixes: tag) is that
>>> as sd_state argument to v4l2_subdev_get_try_crop() et al is NULL, there is
>>> now an attempt to dereference that.
>>>
>>> Fix this.
>>
>> Thank you for this fix, this however only addresses
>> imgu_subdev_get_selection(), but the issue is also present in
>> imgu_subdev_set_selection(). I assume that a similar fix is needed for that.
>> I've added a diff for that below. Feel free to squash that into your patch or
>> let me know if I should submit this separately.
> 
> This looks like a problem indeed. I'll let Sakari decide what he wants
> to do. Adding wrappers along the lines of
> 
> static struct v4l2_rect *
> imgu_subdev_get_crop(struct imgu_v4l2_subdev *sd,
> 		     struct v4l2_subdev_state *sd_state, unsigned int pad,
> 		     enum v4l2_subdev_format_whence which)
> {
> 	if (which == V4L2_SUBDEV_FORMAT_TRY)
> 		return v4l2_subdev_get_try_crop(&imgu_sd->subdev, sd_state, pad);
> 	else
> 		return &imgu_sd->rect.eff;
> }
> 
> (same for the selection rectangle) and using them through the code may
> help.

Thanks! That does seem a good idea. I'll include those in the patch.

Regards,
Max

> 
>> ---
>>    drivers/staging/media/ipu3/ipu3-v4l2.c | 25 ++++++++++++-------------
>>    1 file changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
>> index 2234bb8d48b3..079f2635c70d 100644
>> --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
>> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
>> @@ -223,10 +223,9 @@ static int imgu_subdev_set_selection(struct v4l2_subdev *sd,
>>                                        struct v4l2_subdev_selection *sel)
>>    {
>>           struct imgu_device *imgu = v4l2_get_subdevdata(sd);
>> -       struct imgu_v4l2_subdev *imgu_sd = container_of(sd,
>> -                                                       struct imgu_v4l2_subdev,
>> -                                                       subdev);
>> -       struct v4l2_rect *rect, *try_sel;
>> +       struct imgu_v4l2_subdev *imgu_sd =
>> +               container_of(sd, struct imgu_v4l2_subdev, subdev);
>> +       struct v4l2_rect *rect;
>>
>>           dev_dbg(&imgu->pci_dev->dev,
>>                    "set subdev %u sel which %u target 0x%4x rect [%ux%u]",
>> @@ -238,22 +237,22 @@ static int imgu_subdev_set_selection(struct v4l2_subdev *sd,
>>
>>           switch (sel->target) {
>>           case V4L2_SEL_TGT_CROP:
>> -               try_sel = v4l2_subdev_get_try_crop(sd, sd_state, sel->pad);
>> -               rect = &imgu_sd->rect.eff;
>> +               if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
>> +                       rect = v4l2_subdev_get_try_crop(sd, sd_state, sel->pad);
>> +               else
>> +                       rect = &imgu_sd->rect.eff;
>>                   break;
>>           case V4L2_SEL_TGT_COMPOSE:
>> -               try_sel = v4l2_subdev_get_try_compose(sd, sd_state, sel->pad);
>> -               rect = &imgu_sd->rect.bds;
>> +               if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
>> +                       rect = v4l2_subdev_get_try_compose(sd, sd_state, sel->pad);
>> +               else
>> +                       rect = &imgu_sd->rect.bds;
>>                   break;
>>           default:
>>                   return -EINVAL;
>>           }
>>
>> -       if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
>> -               *try_sel = sel->r;
>> -       else
>> -               *rect = sel->r;
>> -
>> +       *rect = sel->r;
>>           return 0;
>>    }
>>
>