From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx1.redhat.com ([209.132.183.28]:45705 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750925AbbJCIOV (ORCPT ); Sat, 3 Oct 2015 04:14:21 -0400 Subject: Re: [PATCH] [media] gspca: ov534/topro: prevent a division by 0 References: <1443817993-32406-1-git-send-email-ao2@ao2.it> Cc: Antonio Ospite , Linux Media Mailing List , moinejf@free.fr, Anders Blomdell , Thomas Champagne , stable@vger.kernel.org To: Mauro Carvalho Chehab From: Hans de Goede Message-ID: <560F8E59.4090104@redhat.com> Date: Sat, 3 Oct 2015 10:14:17 +0200 MIME-Version: 1.0 In-Reply-To: <1443817993-32406-1-git-send-email-ao2@ao2.it> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Hi, On 02-10-15 22:33, Antonio Ospite wrote: > v4l2-compliance sends a zeroed struct v4l2_streamparm in > v4l2-test-formats.cpp::testParmType(), and this results in a division by > 0 in some gspca subdrivers: > > divide error: 0000 [#1] SMP > Modules linked in: gspca_ov534 gspca_main ... > CPU: 0 PID: 17201 Comm: v4l2-compliance Not tainted 4.3.0-rc2-ao2 #1 > Hardware name: System manufacturer System Product Name/M2N-E SLI, BIOS > ASUS M2N-E SLI ACPI BIOS Revision 1301 09/16/2010 > task: ffff8800818306c0 ti: ffff880095c4c000 task.ti: ffff880095c4c000 > RIP: 0010:[] [] sd_set_streamparm+0x12/0x60 [gspca_ov534] > RSP: 0018:ffff880095c4fce8 EFLAGS: 00010296 > RAX: 0000000000000000 RBX: ffff8800c9522000 RCX: ffffffffa077a140 > RDX: 0000000000000000 RSI: ffff880095e0c100 RDI: ffff8800c9522000 > RBP: ffff880095e0c100 R08: ffffffffa077a100 R09: 00000000000000cc > R10: ffff880067ec7740 R11: 0000000000000016 R12: ffffffffa07bb400 > R13: 0000000000000000 R14: ffff880081b6a800 R15: 0000000000000000 > FS: 00007fda0de78740(0000) GS:ffff88012fc00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00000000014630f8 CR3: 00000000cf349000 CR4: 00000000000006f0 > Stack: > ffffffffa07a6431 ffff8800c9522000 ffffffffa077656e 00000000c0cc5616 > ffff8800c9522000 ffffffffa07a5e20 ffff880095e0c100 0000000000000000 > ffff880067ec7740 ffffffffa077a140 ffff880067ec7740 0000000000000016 > Call Trace: > [] ? v4l_s_parm+0x21/0x50 [videodev] > [] ? vidioc_s_parm+0x4e/0x60 [gspca_main] > [] ? __video_do_ioctl+0x280/0x2f0 [videodev] > [] ? video_ioctl2+0x20/0x20 [videodev] > [] ? video_usercopy+0x319/0x4e0 [videodev] > [] ? page_add_new_anon_rmap+0x71/0xa0 > [] ? mem_cgroup_commit_charge+0x52/0x90 > [] ? handle_mm_fault+0xc18/0x1680 > [] ? v4l2_ioctl+0xac/0xd0 [videodev] > [] ? do_vfs_ioctl+0x28f/0x480 > [] ? SyS_ioctl+0x74/0x80 > [] ? entry_SYSCALL_64_fastpath+0x16/0x75 > Code: c7 93 d9 79 a0 5b 5d e9 f1 f3 9a e0 0f 1f 00 66 2e 0f 1f 84 00 > 00 00 00 00 66 66 66 66 90 53 31 d2 48 89 fb 48 83 ec 08 8b 46 10 > 76 0c 80 bf ac 0c 00 00 00 88 87 4e 0e 00 00 74 09 80 bf 4f > RIP [] sd_set_streamparm+0x12/0x60 [gspca_ov534] > RSP > ---[ end trace 279710c2c6c72080 ]--- > > Following what the doc says about a zeroed timeperframe (see > http://www.linuxtv.org/downloads/v4l-dvb-apis/vidioc-g-parm.html): > > ... > To reset manually applications can just set this field to zero. > > fix the issue by resetting the frame rate to a default value in case of > an unusable timeperframe. > > The fix is done in the subdrivers instead of gspca.c because only the > subdrivers have notion of a default frame rate to reset the camera to. > > Signed-off-by: Antonio Ospite > Cc: stable@vger.kernel.org Good catch: Reviewed-by: Hans de Goede Mauro can you pick this one up directly, and include it in your next pull-req for 4.3 please ? Regards, Hans > --- > > Hi, > > I think the problem in the gspca subdrivers has always been there, so the > patch could be applied to any relevant stable releases. > > After the fix, v4l2-compliance runs fine but it gets two failures, I'll send > another mail about those. > > After this change gets merged I will also send a patch to use defines for the > default framerates used below as the same value is used in multiple places. > > Ah, I ran the patch through scripts/checkpatch.pl from 4.3-rc2 and it > complained about the commit message but I think it may be a false positive. > > Ciao ciao, > Antonio > > > drivers/media/usb/gspca/ov534.c | 9 +++++++-- > drivers/media/usb/gspca/topro.c | 6 +++++- > 2 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/usb/gspca/ov534.c b/drivers/media/usb/gspca/ov534.c > index 146071b..bfff1d1 100644 > --- a/drivers/media/usb/gspca/ov534.c > +++ b/drivers/media/usb/gspca/ov534.c > @@ -1491,8 +1491,13 @@ static void sd_set_streamparm(struct gspca_dev *gspca_dev, > struct v4l2_fract *tpf = &cp->timeperframe; > struct sd *sd = (struct sd *) gspca_dev; > > - /* Set requested framerate */ > - sd->frame_rate = tpf->denominator / tpf->numerator; > + if (tpf->numerator == 0 || tpf->denominator == 0) > + /* Set default framerate */ > + sd->frame_rate = 30; > + else > + /* Set requested framerate */ > + sd->frame_rate = tpf->denominator / tpf->numerator; > + > if (gspca_dev->streaming) > set_frame_rate(gspca_dev); > > diff --git a/drivers/media/usb/gspca/topro.c b/drivers/media/usb/gspca/topro.c > index c70ff40..c028a5c 100644 > --- a/drivers/media/usb/gspca/topro.c > +++ b/drivers/media/usb/gspca/topro.c > @@ -4802,7 +4802,11 @@ static void sd_set_streamparm(struct gspca_dev *gspca_dev, > struct v4l2_fract *tpf = &cp->timeperframe; > int fr, i; > > - sd->framerate = tpf->denominator / tpf->numerator; > + if (tpf->numerator == 0 || tpf->denominator == 0) > + sd->framerate = 30; > + else > + sd->framerate = tpf->denominator / tpf->numerator; > + > if (gspca_dev->streaming) > setframerate(gspca_dev, v4l2_ctrl_g_ctrl(gspca_dev->exposure)); > >