Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] media: gspca: stv06xx: Fix memleak in stv06xx subdrivers
To:     Shuah Khan <skhan@linuxfoundation.org>,
        Pavel Skripkin <paskripkin@gmail.com>
Cc:     Atul Gopinathan <atulgopinathan@gmail.com>,
        syzbot+990626a4ef6f043ed4cd@syzkaller.appspotmail.com,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org,
        mchehab@kernel.org, linux-kernel-mentees@lists.linuxfoundation.org,
        linux-media@vger.kernel.org
References: <20210422160742.7166-1-atulgopinathan@gmail.com>
 <20210422215511.01489adb@gmail.com>
 <36f126fc-6a5e-a078-4cf0-c73d6795a111@linuxfoundation.org>
 <20210423234458.3f754de2@gmail.com>
 <4c22cfa5-5702-6181-0f9a-d1d8d4041156@linuxfoundation.org>
From:   Hans Verkuil <hverkuil-cisco@xs4all.nl>
Message-ID: <e8f45770-76c1-22d3-0960-03e2965b79ab@xs4all.nl>
Date:   Wed, 5 May 2021 11:11:03 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.9.0
MIME-Version: 1.0
In-Reply-To: <4c22cfa5-5702-6181-0f9a-d1d8d4041156@linuxfoundation.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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
>