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.
I added Hans to CC, maybe, he will help :)
With regards,
Pavel Skripkin
