Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754767AbcKJIkz convert rfc822-to-8bit (ORCPT ); Thu, 10 Nov 2016 03:40:55 -0500 Received: from ec2-52-27-115-49.us-west-2.compute.amazonaws.com ([52.27.115.49]:45125 "EHLO osg.samsung.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752287AbcKJIky (ORCPT ); Thu, 10 Nov 2016 03:40:54 -0500 Date: Thu, 10 Nov 2016 06:40:47 -0200 From: Mauro Carvalho Chehab To: Linus Torvalds Cc: =?UTF-8?B?SsO2cmc=?= Otte , Patrick Boettcher , Mauro Carvalho Chehab , Linux Kernel Mailing List , Linux Media Mailing List Subject: Re: [v4.9-rc4] dvb-usb/cinergyT2 NULL pointer dereference Message-ID: <20161110064047.1e0a6b4b@vento.lan> In-Reply-To: References: <20161108182215.41f1f3d2@vento.lan> Organization: Samsung X-Mailer: Claws Mail 3.14.0 (GTK+ 2.24.31; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8025 Lines: 207 Em Wed, 9 Nov 2016 11:07:35 -0800 Linus Torvalds escreveu: > On Wed, Nov 9, 2016 at 3:09 AM, Jörg Otte wrote: > > > > Tried patch with no success. Again a NULL ptr dereferece. > > That patch was pure garbage, I think. Pretty much all the other > drivers that use the same approach will have the same issue. Adding > that init function just for the semaphore is crazy. > > I suspect a much simpler approach is to just miove the "data_mutex" > away from the priv area and into "struct dvb_usb_device" and > "dvb_usb_adapter". Sure, that grows those structures a tiny bit, and > not every driver may need that mutex, but it simplifies things > enormously. Mauro? > > Linus [PATCH] cinergyT2-core: move data_mutex to struct dvb_usb_device The data_mutex is initialized too late, as it is needed for the device's power control, causing an OOPS: dvb-usb: found a 'TerraTec/qanu USB2.0 Highspeed DVB-T Receiver' in warm state. BUG: unable to handle kernel NULL pointer dereference at (null) IP: [] __mutex_lock_slowpath+0x6f/0x100 PGD 0 Oops: 0002 [#1] SMP Modules linked in: dvb_usb_cinergyT2(+) dvb_usb CPU: 0 PID: 2029 Comm: modprobe Not tainted 4.9.0-rc4-dvbmod #24 Hardware name: FUJITSU LIFEBOOK A544/FJNBB35 , BIOS Version 1.17 05/09/2014 task: ffff88020e943840 task.stack: ffff8801f36ec000 RIP: 0010:[] [] __mutex_lock_slowpath+0x6f/0x100 RSP: 0018:ffff8801f36efb10 EFLAGS: 00010282 RAX: 0000000000000000 RBX: ffff88021509bdc8 RCX: 00000000c0000100 RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff88021509bdcc RBP: ffff8801f36efb58 R08: ffff88021f216320 R09: 0000000000100000 R10: ffff88021f216320 R11: 00000023fee6c5a1 R12: ffff88020e943840 R13: ffff88021509bdcc R14: 00000000ffffffff R15: ffff88021509bdd0 FS: 00007f21adb86740(0000) GS:ffff88021f200000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000000000 CR3: 0000000215bce000 CR4: 00000000001406f0 Stack: ffff88021509bdd0 0000000000000000 0000000000000000 ffffffffc0137c80 ffff88021509bdc8 ffff8801f5944000 0000000000000001 ffffffffc0136b00 ffff880213e52000 ffff88021509bdc8 ffffffff84661856 ffff88021509bd80 Call Trace: [] ? mutex_lock+0x16/0x25 [] ? cinergyt2_power_ctrl+0x1f/0x60 [dvb_usb_cinergyT2] [] ? dvb_usb_device_init+0x21e/0x5d0 [dvb_usb] [] ? cinergyt2_usb_probe+0x21/0x50 [dvb_usb_cinergyT2] [] ? usb_probe_interface+0xf3/0x2a0 [] ? driver_probe_device+0x208/0x2b0 [] ? __driver_attach+0x87/0x90 [] ? driver_probe_device+0x2b0/0x2b0 [] ? bus_for_each_dev+0x52/0x80 [] ? bus_add_driver+0x1a3/0x220 [] ? driver_register+0x56/0xd0 [] ? usb_register_driver+0x77/0x130 [] ? 0xffffffffc013a000 [] ? do_one_initcall+0x46/0x180 [] ? free_vmap_area_noflush+0x38/0x70 [] ? kmem_cache_alloc+0x84/0xc0 [] ? do_init_module+0x50/0x1be [] ? load_module+0x1d8b/0x2100 [] ? find_symbol_in_section+0xa0/0xa0 [] ? SyS_finit_module+0x89/0x90 [] ? entry_SYSCALL_64_fastpath+0x13/0x94 Code: e8 a7 1d 00 00 8b 03 83 f8 01 0f 84 97 00 00 00 48 8b 43 10 4c 8d 7b 08 48 89 63 10 4c 89 3c 24 41 be ff ff ff ff 48 89 44 24 08 <48> 89 20 4c 89 64 24 10 eb 1a 49 c7 44 24 08 02 00 00 00 c6 43 RIP [] __mutex_lock_slowpath+0x6f/0x100 RSP CR2: 0000000000000000 So, move it to the struct dvb_usb_device and initialize it before calling the driver's callbacks. Reported-by: Jörg Otte Signed-off-by: Mauro Carvalho Chehab diff --git a/drivers/media/usb/dvb-usb/cinergyT2-core.c b/drivers/media/usb/dvb-usb/cinergyT2-core.c index 8ac825413d5a..87e3bd33900d 100644 --- a/drivers/media/usb/dvb-usb/cinergyT2-core.c +++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c @@ -42,7 +42,6 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr); struct cinergyt2_state { u8 rc_counter; unsigned char data[64]; - struct mutex data_mutex; }; /* We are missing a release hook with usb_device data */ @@ -56,12 +55,12 @@ static int cinergyt2_streaming_ctrl(struct dvb_usb_adapter *adap, int enable) struct cinergyt2_state *st = d->priv; int ret; - mutex_lock(&st->data_mutex); + mutex_lock(&d->data_mutex); st->data[0] = CINERGYT2_EP1_CONTROL_STREAM_TRANSFER; st->data[1] = enable ? 1 : 0; ret = dvb_usb_generic_rw(d, st->data, 2, st->data, 64, 0); - mutex_unlock(&st->data_mutex); + mutex_unlock(&d->data_mutex); return ret; } @@ -71,12 +70,12 @@ static int cinergyt2_power_ctrl(struct dvb_usb_device *d, int enable) struct cinergyt2_state *st = d->priv; int ret; - mutex_lock(&st->data_mutex); + mutex_lock(&d->data_mutex); st->data[0] = CINERGYT2_EP1_SLEEP_MODE; st->data[1] = enable ? 0 : 1; ret = dvb_usb_generic_rw(d, st->data, 2, st->data, 3, 0); - mutex_unlock(&st->data_mutex); + mutex_unlock(&d->data_mutex); return ret; } @@ -89,7 +88,7 @@ static int cinergyt2_frontend_attach(struct dvb_usb_adapter *adap) adap->fe_adap[0].fe = cinergyt2_fe_attach(adap->dev); - mutex_lock(&st->data_mutex); + mutex_lock(&d->data_mutex); st->data[0] = CINERGYT2_EP1_GET_FIRMWARE_VERSION; ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 3, 0); @@ -97,7 +96,7 @@ static int cinergyt2_frontend_attach(struct dvb_usb_adapter *adap) deb_rc("cinergyt2_power_ctrl() Failed to retrieve sleep " "state info\n"); } - mutex_unlock(&st->data_mutex); + mutex_unlock(&d->data_mutex); /* Copy this pointer as we are gonna need it in the release phase */ cinergyt2_usb_device = adap->dev; @@ -166,7 +165,7 @@ static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state) *state = REMOTE_NO_KEY_PRESSED; - mutex_lock(&st->data_mutex); + mutex_lock(&d->data_mutex); st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS; ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0); @@ -202,7 +201,7 @@ static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state) } ret: - mutex_unlock(&st->data_mutex); + mutex_unlock(&d->data_mutex); return ret; } @@ -210,7 +209,6 @@ static int cinergyt2_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) { struct dvb_usb_device *d; - struct cinergyt2_state *st; int ret; ret = dvb_usb_device_init(intf, &cinergyt2_properties, @@ -218,9 +216,6 @@ static int cinergyt2_usb_probe(struct usb_interface *intf, if (ret < 0) return ret; - st = d->priv; - mutex_init(&st->data_mutex); - return 0; } diff --git a/drivers/media/usb/dvb-usb/dvb-usb-init.c b/drivers/media/usb/dvb-usb/dvb-usb-init.c index 3896ba9a4179..84308569e7dc 100644 --- a/drivers/media/usb/dvb-usb/dvb-usb-init.c +++ b/drivers/media/usb/dvb-usb/dvb-usb-init.c @@ -142,6 +142,7 @@ static int dvb_usb_init(struct dvb_usb_device *d, short *adapter_nums) { int ret = 0; + mutex_init(&d->data_mutex); mutex_init(&d->usb_mutex); mutex_init(&d->i2c_mutex); diff --git a/drivers/media/usb/dvb-usb/dvb-usb.h b/drivers/media/usb/dvb-usb/dvb-usb.h index 1448c3d27ea2..12b71acee550 100644 --- a/drivers/media/usb/dvb-usb/dvb-usb.h +++ b/drivers/media/usb/dvb-usb/dvb-usb.h @@ -404,6 +404,7 @@ struct dvb_usb_adapter { * Powered is in/decremented for each call to modify the state. * @udev: pointer to the device's struct usb_device. * + * @data_mutex: mutex to protect the data structure used to store URB data * @usb_mutex: semaphore of USB control messages (reading needs two messages) * @i2c_mutex: semaphore for i2c-transfers * @@ -433,6 +434,7 @@ struct dvb_usb_device { int powered; /* locking */ + struct mutex data_mutex; struct mutex usb_mutex; /* i2c */