From: Shuah Khan <skhan@linuxfoundation.org>
To: Pavel Skripkin <paskripkin@gmail.com>, hverkuil-cisco@xs4all.nl
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,
Shuah Khan <skhan@linuxfoundation.org>
Subject: Re: [PATCH] media: gspca: stv06xx: Fix memleak in stv06xx subdrivers
Date: Fri, 23 Apr 2021 14:56:26 -0600 [thread overview]
Message-ID: <4c22cfa5-5702-6181-0f9a-d1d8d4041156@linuxfoundation.org> (raw)
In-Reply-To: <20210423234458.3f754de2@gmail.com>
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.
thanks,
-- Shuah
next prev parent reply other threads:[~2021-04-23 20:56 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-22 16:07 [PATCH] media: gspca: stv06xx: Fix memleak in stv06xx subdrivers Atul Gopinathan
2021-04-22 18:55 ` Pavel Skripkin
2021-04-23 3:18 ` Atul Gopinathan
2021-04-23 20:19 ` Shuah Khan
2021-04-23 20:44 ` Pavel Skripkin
2021-04-23 20:56 ` Shuah Khan [this message]
2021-05-05 9:11 ` Hans Verkuil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4c22cfa5-5702-6181-0f9a-d1d8d4041156@linuxfoundation.org \
--to=skhan@linuxfoundation.org \
--cc=atulgopinathan@gmail.com \
--cc=hverkuil-cisco@xs4all.nl \
--cc=linux-kernel-mentees@lists.linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=paskripkin@gmail.com \
--cc=stable@vger.kernel.org \
--cc=syzbot+990626a4ef6f043ed4cd@syzkaller.appspotmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).