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=ab69427f2911374e5f0b347d0d7795bfe384016...
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=4f4e6644cd876c844cdb3bea2d...
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.
thanks, -- Shuah