Re: [PATCH] media: gspca: stv06xx: Fix memleak in stv06xx subdrivers

[Hans Verkuil <hverkuil-cisco@xs4all.nl>]

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@linuxfoundation.org> wrote:
>>> On 4/22/21 12:55 PM, Pavel Skripkin wrote:
>>>> Hi!
>>>>
>>>> On Thu, 22 Apr 2021 21:37:42 +0530
>>>> Atul Gopinathan <atulgopinathan@gmail.com> 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@vger.kernel.org
>>>>> Suggested-by: Shuah Khan <skhan@linuxfoundation.org>
>>>>> Reported-by: syzbot+990626a4ef6f043ed4cd@syzkaller.appspotmail.com
>>>>> Tested-by: syzbot+990626a4ef6f043ed4cd@syzkaller.appspotmail.com
>>>>> Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com>
>>>>
>>>> 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
>