| Message ID | 20210422160742.7166-1-atulgopinathan@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: gspca: stv06xx: Fix memleak in stv06xx subdrivers | expand |
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
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 >
diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c index 158c8e28ed2c..88298d9f604d 100644 --- a/drivers/media/usb/gspca/gspca.c +++ b/drivers/media/usb/gspca/gspca.c @@ -1577,6 +1577,8 @@ int gspca_dev_probe2(struct usb_interface *intf, v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler); v4l2_device_unregister(&gspca_dev->v4l2_dev); kfree(gspca_dev->usb_buf); + if (sd_desc->free_sensor_priv) + sd_desc->free_sensor_priv(gspca_dev); kfree(gspca_dev); return ret; } diff --git a/drivers/media/usb/gspca/gspca.h b/drivers/media/usb/gspca/gspca.h index b0ced2e14006..792f19307b91 100644 --- a/drivers/media/usb/gspca/gspca.h +++ b/drivers/media/usb/gspca/gspca.h @@ -119,6 +119,7 @@ struct sd_desc { cam_streamparm_op set_streamparm; cam_format_op try_fmt; cam_frmsize_op enum_framesizes; + cam_op free_sensor_priv; #ifdef CONFIG_VIDEO_ADV_DEBUG cam_set_reg_op set_register; cam_get_reg_op get_register; diff --git a/drivers/media/usb/gspca/stv06xx/stv06xx.c b/drivers/media/usb/gspca/stv06xx/stv06xx.c index 95673fc0a99c..de1daeb1a03c 100644 --- a/drivers/media/usb/gspca/stv06xx/stv06xx.c +++ b/drivers/media/usb/gspca/stv06xx/stv06xx.c @@ -529,6 +529,8 @@ static int sd_int_pkt_scan(struct gspca_dev *gspca_dev, static int stv06xx_config(struct gspca_dev *gspca_dev, const struct usb_device_id *id); +static int stv06xx_free_sensor_priv(struct gspca_dev *gspca_dev); + /* sub-driver description */ static const struct sd_desc sd_desc = { .name = MODULE_NAME, @@ -540,6 +542,7 @@ static const struct sd_desc sd_desc = { .pkt_scan = stv06xx_pkt_scan, .isoc_init = stv06xx_isoc_init, .isoc_nego = stv06xx_isoc_nego, + .free_sensor_priv = stv06xx_free_sensor_priv, #if IS_ENABLED(CONFIG_INPUT) .int_pkt_scan = sd_int_pkt_scan, #endif @@ -583,7 +586,19 @@ static int stv06xx_config(struct gspca_dev *gspca_dev, return -ENODEV; } +/* + * Free the memory allocated to sd->sensor_priv field during initial phases of + * gspca (probe/init_control) which later encountered error in the same phase. + */ +static int stv06xx_free_sensor_priv(struct gspca_dev *gspca_dev) +{ + struct sd *sd = (struct sd *) gspca_dev; + + if (sd->sensor->free_sensor_priv) + sd->sensor->free_sensor_priv(sd); + return 0; +} /* -- module initialisation -- */ static const struct usb_device_id device_table[] = { diff --git a/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c b/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c index 5a47dcbf1c8e..50b7a80a855a 100644 --- a/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c +++ b/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c @@ -529,3 +529,12 @@ static int hdcs_dump(struct sd *sd) } return 0; } + +static int hdcs_deallocate(struct sd *sd) +{ + struct hdcs *hdcs = sd->sensor_priv; + + kfree(hdcs); + sd->sensor_priv = NULL; + return 0; +} diff --git a/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.h b/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.h index 326a6eb68237..482e1a8e9a3b 100644 --- a/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.h +++ b/drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.h @@ -121,6 +121,7 @@ static int hdcs_init(struct sd *sd); static int hdcs_init_controls(struct sd *sd); static int hdcs_stop(struct sd *sd); static int hdcs_dump(struct sd *sd); +static int hdcs_deallocate(struct sd *sd); static int hdcs_set_exposure(struct gspca_dev *gspca_dev, __s32 val); static int hdcs_set_gain(struct gspca_dev *gspca_dev, __s32 val); @@ -142,6 +143,7 @@ const struct stv06xx_sensor stv06xx_sensor_hdcs1x00 = { .start = hdcs_start, .stop = hdcs_stop, .dump = hdcs_dump, + .free_sensor_priv = hdcs_deallocate, }; const struct stv06xx_sensor stv06xx_sensor_hdcs1020 = { @@ -161,6 +163,7 @@ const struct stv06xx_sensor stv06xx_sensor_hdcs1020 = { .start = hdcs_start, .stop = hdcs_stop, .dump = hdcs_dump, + .free_sensor_priv = hdcs_deallocate, }; static const u16 stv_bridge_init[][2] = { diff --git a/drivers/media/usb/gspca/stv06xx/stv06xx_pb0100.c b/drivers/media/usb/gspca/stv06xx/stv06xx_pb0100.c index ae382b3b5f7f..97ea886698c1 100644 --- a/drivers/media/usb/gspca/stv06xx/stv06xx_pb0100.c +++ b/drivers/media/usb/gspca/stv06xx/stv06xx_pb0100.c @@ -318,6 +318,15 @@ static int pb0100_dump(struct sd *sd) return 0; } +static int pb0100_free_ctrls(struct sd *sd) +{ + struct pb0100_ctrls *ctrls = sd->sensor_priv; + + kfree(ctrls); + sd->sensor_priv = NULL; + return 0; +} + static int pb0100_set_gain(struct gspca_dev *gspca_dev, __s32 val) { int err; diff --git a/drivers/media/usb/gspca/stv06xx/stv06xx_pb0100.h b/drivers/media/usb/gspca/stv06xx/stv06xx_pb0100.h index c5a6614577de..12ec207f6bfa 100644 --- a/drivers/media/usb/gspca/stv06xx/stv06xx_pb0100.h +++ b/drivers/media/usb/gspca/stv06xx/stv06xx_pb0100.h @@ -102,6 +102,7 @@ static int pb0100_init(struct sd *sd); static int pb0100_init_controls(struct sd *sd); static int pb0100_stop(struct sd *sd); static int pb0100_dump(struct sd *sd); +static int pb0100_free_ctrls(struct sd *sd); /* V4L2 controls supported by the driver */ static int pb0100_set_gain(struct gspca_dev *gspca_dev, __s32 val); @@ -126,6 +127,7 @@ const struct stv06xx_sensor stv06xx_sensor_pb0100 = { .start = pb0100_start, .stop = pb0100_stop, .dump = pb0100_dump, + .free_sensor_priv = pb0100_free_ctrls, }; #endif diff --git a/drivers/media/usb/gspca/stv06xx/stv06xx_sensor.h b/drivers/media/usb/gspca/stv06xx/stv06xx_sensor.h index 67c13c689b9c..22c75c528331 100644 --- a/drivers/media/usb/gspca/stv06xx/stv06xx_sensor.h +++ b/drivers/media/usb/gspca/stv06xx/stv06xx_sensor.h @@ -69,6 +69,9 @@ struct stv06xx_sensor { /* Instructs the sensor to dump all its contents */ int (*dump)(struct sd *sd); + + /* Frees sensor_priv field during initial gspca probe errors */ + int (*free_sensor_priv)(struct sd *sd); }; #endif
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> --- drivers/media/usb/gspca/gspca.c | 2 ++ drivers/media/usb/gspca/gspca.h | 1 + drivers/media/usb/gspca/stv06xx/stv06xx.c | 15 +++++++++++++++ drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.c | 9 +++++++++ drivers/media/usb/gspca/stv06xx/stv06xx_hdcs.h | 3 +++ drivers/media/usb/gspca/stv06xx/stv06xx_pb0100.c | 9 +++++++++ drivers/media/usb/gspca/stv06xx/stv06xx_pb0100.h | 2 ++ drivers/media/usb/gspca/stv06xx/stv06xx_sensor.h | 3 +++ 8 files changed, 44 insertions(+)