| Message ID | 547CF9FC.5010101@southpole.se (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1XvaQ0-00069d-Mf; Tue, 02 Dec 2014 00:30:12 +0100 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.72/mailfrontend-8) with esmtp id 1XvaPy-0004s3-lN; Tue, 02 Dec 2014 00:30:12 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754193AbaLAXaI (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Mon, 1 Dec 2014 18:30:08 -0500 Received: from smtp.bredband2.com ([83.219.192.166]:47818 "EHLO smtp.bredband2.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753571AbaLAXaH (ORCPT <rfc822; linux-media@vger.kernel.org>); Mon, 1 Dec 2014 18:30:07 -0500 Received: from [192.168.1.22] (92-244-23-216.customers.ownit.se [92.244.23.216]) (Authenticated sender: ed8153) by smtp.bredband2.com (Postfix) with ESMTPA id ADD21145687; Tue, 2 Dec 2014 00:30:04 +0100 (CET) Message-ID: <547CF9FC.5010101@southpole.se> Date: Tue, 02 Dec 2014 00:30:04 +0100 From: Benjamin Larsson <benjamin@southpole.se> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: linux-media@vger.kernel.org, Antti Palosaari <crope@iki.fi> Subject: Re: Random memory corruption of fe[1]->dvb pointer References: <547BAC79.50702@southpole.se> In-Reply-To: <547BAC79.50702@southpole.se> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2014.12.1.231524 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' HTML_00_01 0.05, HTML_00_10 0.05, MSGID_ADDED_BY_MTA 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_2000_2999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, REFERENCES 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __BOUNCE_CHALLENGE_SUBJ 0, __BOUNCE_NDR_SUBJ_EXEMPT 0, __CP_URI_IN_BODY 0, __CT 0, __CTE 0, __CT_TEXT_PLAIN 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILING_LIST 0, __IN_REP_TO 0, __MIME_TEXT_ONLY 0, __MIME_VERSION 0, __MOZILLA_MSGID 0, __MOZILLA_USER_AGENT 0, __REFERENCES 0, __SANE_MSGID 0, __SUBJ_ALPHA_END 0, __SUBJ_ALPHA_NEGATE 0, __TO_MALFORMED_2 0, __TO_NO_NAME 0, __URI_NO_WWW 0, __URI_NS , __USER_AGENT 0' |
Commit Message
Benjamin Larsson
Dec. 1, 2014, 11:30 p.m. UTC
I think I have found the issue for this error and it looks like a use after free that affects multiple drivers. The effect is that the driver crashes on unload. I added the following code to the mn88472 driver, it should behave as a nop: When I now unload the driver I get the following code flow: usb 1-1: rtl28xxu_exit: mn88472 2-0018: mn88472_remove: <-- this call will actually free the fe[1] pointer, I added the memset to make sure they where null usb 1-1: dvb_usbv2_exit: usb 1-1: dvb_usbv2_remote_exit: usb 1-1: dvb_usbv2_adapter_exit: usb 1-1: dvb_usbv2_adapter_exit: fe0[0]=0xffff88007a8b0018 usb 1-1: dvb_usbv2_adapter_exit: fe0[0]->dvb=0xffff88007a142580 usb 1-1: dvb_usbv2_adapter_exit: fe0[0]->demodulator_priv=0xffff88007a8b0000 usb 1-1: dvb_usbv2_adapter_exit: fe1[0]=0xffff88007a8d0030 usb 1-1: dvb_usbv2_adapter_exit: fe1[0]->dvb=0x (null) usb 1-1: dvb_usbv2_adapter_exit: fe1[0]->demodulator_priv=0x (null) BUG: unable to handle kernel NULL pointer dereference at 0000000000000040 IP: [<ffffffffa021f3de>] dvb_unregister_frontend+0x2a/0xf1 [dvb_core] dvb_unregister_frontend() is sent the fe[1] pointer which now is null and thus crashes with a null pointer dereference. A use after free issue. I looked for similar code and found it in: si2168.c af9033.c tc90522.c sp2.c has the same structure but I think it is fine. So at first it would be nice if someone could confirm my findings. Applying the same kind of code like my patch and unplug something that uses the affected frontend should be enough. MvH Benjamin Larsson -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Comments
> So at first it would be nice if someone could confirm my findings. > Applying the same kind of code like my patch and unplug something that > uses the affected frontend should be enough. I tried that for tc90522, and I could remove earth-pt3 (which uses tc90522), tc90522 and tuner modules without any problem, although earth-pt3 is a pci driver and does not use dvb-usb-v2. From your log(?) output, I guess that rtl28xxu_exit() removed the attached demod module (mn88472) and thus free'ed fe BEFORE calling dvb_usbv2_exit(), from where dvb_unregister_frontend(fe) is called. I think that the demod i2c device is removed automatically by dvb_usbv2_i2c_exit() in dvb_usbv2_exit(), if you registered the demod i2c device, and your adapter/bridge driver should not try to remove it. regards, Akihiro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/02/2014 11:47 AM, Akihiro TSUKADA wrote: >> So at first it would be nice if someone could confirm my findings. >> Applying the same kind of code like my patch and unplug something that >> uses the affected frontend should be enough. > > I tried that for tc90522, and I could remove earth-pt3 > (which uses tc90522), tc90522 and tuner modules without any problem, > although earth-pt3 is a pci driver and does not use dvb-usb-v2. > >>From your log(?) output, > I guess that rtl28xxu_exit() removed the attached demod module > (mn88472) and thus free'ed fe BEFORE calling dvb_usbv2_exit(), > from where dvb_unregister_frontend(fe) is called. > I think that the demod i2c device is removed automatically by > dvb_usbv2_i2c_exit() in dvb_usbv2_exit(), if you registered > the demod i2c device, and your adapter/bridge driver > should not try to remove it. Yes. You must unregister frontend before you remove driver. I have already added new callbacks detach tuner and frontend to avoid that, but there was yet again new issue as it removes rtl2832 demod driver first and mn88472 slave demod was put to i2c bus / adapter which is owned by rtl2832. So it will crash too. Solution is to convert rtl2832 to I2C binding (or convert mn88472 legacy DVB binding (which I don't allow :)). When rtl2832 driver is converted to I2C model it is not unloaded automatically and you could remove those in a correct order. But hey, mn88472 is still on staging :D regards Antti
On 2014-12-02 11:02, Antti Palosaari wrote: > > > On 12/02/2014 11:47 AM, Akihiro TSUKADA wrote: >>> So at first it would be nice if someone could confirm my findings. >>> Applying the same kind of code like my patch and unplug something that >>> uses the affected frontend should be enough. >> >> I tried that for tc90522, and I could remove earth-pt3 >> (which uses tc90522), tc90522 and tuner modules without any problem, >> although earth-pt3 is a pci driver and does not use dvb-usb-v2. >> >>> From your log(?) output, >> I guess that rtl28xxu_exit() removed the attached demod module >> (mn88472) and thus free'ed fe BEFORE calling dvb_usbv2_exit(), >> from where dvb_unregister_frontend(fe) is called. >> I think that the demod i2c device is removed automatically by >> dvb_usbv2_i2c_exit() in dvb_usbv2_exit(), if you registered >> the demod i2c device, and your adapter/bridge driver >> should not try to remove it. > > Yes. You must unregister frontend before you remove driver. I have > already added new callbacks detach tuner and frontend to avoid that, > but there was yet again new issue as it removes rtl2832 demod driver > first and mn88472 slave demod was put to i2c bus / adapter which is > owned by rtl2832. So it will crash too. Solution is to convert rtl2832 > to I2C binding (or convert mn88472 legacy DVB binding (which I don't > allow :)). When rtl2832 driver is converted to I2C model it is not > unloaded automatically and you could remove those in a correct order. > > But hey, mn88472 is still on staging :D > > regards > Antti > So the solution is to change rtl2832.c to the I2C model? And does this issue only affect the mn8847x drivers ? If this is the case would a patch that does not free the buffer but leaks the memory be ok ? I can add a todo item and log it in syslog. That would for sure be better then crashing the subsystem and the driver is still in staging for a reason. MvH Benjamin Larsson -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/02/2014 12:41 PM, Benjamin Larsson wrote: > On 2014-12-02 11:02, Antti Palosaari wrote: >> >> >> On 12/02/2014 11:47 AM, Akihiro TSUKADA wrote: >>>> So at first it would be nice if someone could confirm my findings. >>>> Applying the same kind of code like my patch and unplug something that >>>> uses the affected frontend should be enough. >>> >>> I tried that for tc90522, and I could remove earth-pt3 >>> (which uses tc90522), tc90522 and tuner modules without any problem, >>> although earth-pt3 is a pci driver and does not use dvb-usb-v2. >>> >>>> From your log(?) output, >>> I guess that rtl28xxu_exit() removed the attached demod module >>> (mn88472) and thus free'ed fe BEFORE calling dvb_usbv2_exit(), >>> from where dvb_unregister_frontend(fe) is called. >>> I think that the demod i2c device is removed automatically by >>> dvb_usbv2_i2c_exit() in dvb_usbv2_exit(), if you registered >>> the demod i2c device, and your adapter/bridge driver >>> should not try to remove it. >> >> Yes. You must unregister frontend before you remove driver. I have >> already added new callbacks detach tuner and frontend to avoid that, >> but there was yet again new issue as it removes rtl2832 demod driver >> first and mn88472 slave demod was put to i2c bus / adapter which is >> owned by rtl2832. So it will crash too. Solution is to convert rtl2832 >> to I2C binding (or convert mn88472 legacy DVB binding (which I don't >> allow :)). When rtl2832 driver is converted to I2C model it is not >> unloaded automatically and you could remove those in a correct order. >> >> But hey, mn88472 is still on staging :D >> >> regards >> Antti >> > > So the solution is to change rtl2832.c to the I2C model? And does this > issue only affect the mn8847x drivers ? It likely affects some other dvb-usb-v2 drivers too. But not af9035 as I fixed it initially there I think. > If this is the case would a patch that does not free the buffer but > leaks the memory be ok ? I can add a todo item and log it in syslog. > That would for sure be better then crashing the subsystem and the driver > is still in staging for a reason. Maybe yes, but it does not sound absolute any good. I think you will need to set FE pointer NULL after driver is removed. Then unregister frontend will not call members of that struct anymore, but leak memory? regards Antti
On 2014-12-02 11:59, Antti Palosaari wrote: > [...] >> So the solution is to change rtl2832.c to the I2C model? And does this >> issue only affect the mn8847x drivers ? > > It likely affects some other dvb-usb-v2 drivers too. But not af9035 as > I fixed it initially there I think. > >> If this is the case would a patch that does not free the buffer but >> leaks the memory be ok ? I can add a todo item and log it in syslog. >> That would for sure be better then crashing the subsystem and the driver >> is still in staging for a reason. > > Maybe yes, but it does not sound absolute any good. I think you will > need to set FE pointer NULL after driver is removed. It is NULL now, that is why it is crashing, or the current code leads to random corruptions. > Then unregister frontend will not call members of that struct anymore, > but leak memory? Well any solution that does not randomly crash the kernel when unloading the module is fine by me. My suggestion is to leak the memory and put a note about it in syslog. But I guess there are only a handful of users of this driver so maybe leave it as it is right now? It must be fixed anyway before the driver is moved out of staging. > > regards > Antti > MvH Benjamin Larsson -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/staging/media/mn88472/mn88472.c b/drivers/staging/media/mn88472/mn88472.c index 52de8f8..58af319 100644 --- a/drivers/staging/media/mn88472/mn88472.c +++ b/drivers/staging/media/mn88472/mn88472.c @@ -494,6 +494,7 @@ static int mn88472_remove(struct i2c_client *client) regmap_exit(dev->regmap[0]); + memset(dev, 0, sizeof(*dev)); kfree(dev);