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:     Pavel Skripkin <paskripkin@gmail.com>,
        Atul Gopinathan <atulgopinathan@gmail.com>
Cc:     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, Shuah Khan <skhan@linuxfoundation.org>
References: <20210422160742.7166-1-atulgopinathan@gmail.com>
 <20210422215511.01489adb@gmail.com>
From:   Shuah Khan <skhan@linuxfoundation.org>
Message-ID: <36f126fc-6a5e-a078-4cf0-c73d6795a111@linuxfoundation.org>
Date:   Fri, 23 Apr 2021 14:19:15 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.1
MIME-Version: 1.0
In-Reply-To: <20210422215511.01489adb@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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