On 23/04/2021 22:56, Shuah Khan wrote:
> On 4/23/21 2:44 PM, Pavel Skripkin wrote:
>> Hi!
>>
>> On Fri, 23 Apr 2021 14:19:15 -0600
>> Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx> wrote:
>>> On 4/22/21 12:55 PM, Pavel Skripkin wrote:
>>>> Hi!
>>>>
>>>> On Thu, 22 Apr 2021 21:37:42 +0530
>>>> Atul Gopinathan <atulgopinathan@xxxxxxxxx> wrote:
>>>>> During probing phase of a gspca driver in "gspca_dev_probe2()", the
>>>>> stv06xx subdrivers have certain sensor variants (namely, hdcs_1x00,
>>>>> hdcs_1020 and pb_0100) that allocate memory for their respective
>>>>> sensor which is passed to the "sd->sensor_priv" field. During the
>>>>> same probe routine, after "sensor_priv" allocation, there are
>>>>> chances of later functions invoked to fail which result in the
>>>>> probing routine to end immediately via "goto out" path. While
>>>>> doing so, the memory allocated earlier for the sensor isn't taken
>>>>> care of resulting in memory leak.
>>>>>
>>>>> Fix this by adding operations to the gspca, stv06xx and down to the
>>>>> sensor levels to free this allocated memory during gspca probe
>>>>> failure.
>>>>>
>>>>> -
>>>>> The current level of hierarchy looks something like this:
>>>>>
>>>>> gspca (main driver) represented by struct gspca_dev
>>>>> |
>>>>> ___________|_____________________________________
>>>>> | | | | | | (subdrivers)
>>>>> | represented
>>>>> stv06xx by
>>>>> "struct sd" |
>>>>> _______________|_______________
>>>>> | | | | | (sensors)
>>>>> | |
>>>>> hdcs_1x00/1020 pb01000
>>>>> |_________________|
>>>>> |
>>>>> These three sensor variants
>>>>> allocate memory for
>>>>> "sd->sensor_priv" field.
>>>>>
>>>>> Here, "struct gspca_dev" is the representation used in the top
>>>>> level. In the sub-driver levels, "gspca_dev" pointer is cast to
>>>>> "struct sd*", something like this:
>>>>>
>>>>> struct sd *sd = (struct sd *)gspca_dev;
>>>>>
>>>>> This is possible because the first field of "struct sd" is
>>>>> "gspca_dev":
>>>>>
>>>>> struct sd {
>>>>> struct gspca_dev;
>>>>> .
>>>>> .
>>>>> }
>>>>>
>>>>> Therefore, to deallocate the "sd->sensor_priv" fields from
>>>>> "gspca_dev_probe2()" which is at the top level, the patch creates
>>>>> operations for the subdrivers and sensors to be invoked from the
>>>>> gspca driver levels. These operations essentially free the
>>>>> "sd->sensor_priv" which were allocated by the "config" and
>>>>> "init_controls" operations in the case of stv06xx sub-drivers and
>>>>> the sensor levels.
>>>>>
>>>>> This patch doesn't affect other sub-drivers or even sensors who
>>>>> never allocate memory to "sensor_priv". It has also been tested by
>>>>> syzbot and it returned an "OK" result.
>>>>>
>>>>> https://syzkaller.appspot.com/bug?id=ab69427f2911374e5f0b347d0d7795bfe384016c
>>>>> -
>>>>>
>>>>> Fixes: 4c98834addfe ("V4L/DVB (10048): gspca - stv06xx: New
>>>>> subdriver.") Cc: stable@xxxxxxxxxxxxxxx
>>>>> Suggested-by: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>
>>>>> Reported-by: syzbot+990626a4ef6f043ed4cd@xxxxxxxxxxxxxxxxxxxxxxxxx
>>>>> Tested-by: syzbot+990626a4ef6f043ed4cd@xxxxxxxxxxxxxxxxxxxxxxxxx
>>>>> Signed-off-by: Atul Gopinathan <atulgopinathan@xxxxxxxxx>
>>>>
>>>> AFAIK, something similar is already applied to linux-media tree
>>>> https://git.linuxtv.org/media_tree.git/commit/?id=4f4e6644cd876c844cdb3bea2dd7051787d5ae25
>>>>
>>>
>>> Pavel,
>>>
>>> Does the above handle the other drivers hdcs_1x00/1020 and pb01000?
>>>
>>> Atul's patch handles those cases. If thoese code paths need to be
>>> fixes, Atul could do a patch on top of yours perhaps?
>>>
>>> thanks,
>>> -- Shuah
>>>
>>>
>>
>> It's not my patch. I've sent a patch sometime ago, but it was reject
>> by Mauro (we had a small discussion on linux-media mailing-list), then
>> Hans wrote the patch based on my leak discoverage.
>>
>
> Yes my bad. :)
>
>> I added Hans to CC, maybe, he will help :)
>>
>
> Will wait for Hans to take a look.
Yes, my patch does the same as this patch, just a bit more concise.
I'll drop this one.
Regards,
Hans
>
> thanks,
> -- Shuah
>
