Random memory corruption of fe[1]->dvb pointer

Random memory corruption of fe[1]->dvb pointer

[Benjamin Larsson]

While working on a driver I noticed that I had trouble unloading the 
module after testing, it crashed while running
dvb_usbv2_adapter_frontend_exit. So I added a print out of some pointers 
and got this:

Init:
usb 1-1: dvb_usbv2_adapter_frontend_init: adap=fe[0] ffff88006afa6818
usb 1-1: dvb_usbv2_adapter_frontend_init: adap=fe[0]->dvb ffff880078cba580
usb 1-1: dvb_usbv2_adapter_frontend_init: adap=fe[1] ffff88003698e830
usb 1-1: dvb_usbv2_adapter_frontend_init: adap=fe[1]->dvb ffff880078cba580

ok looking 64bit pointers

Deinit:
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]= ffff88006afa6818
usb 1-1: dvb_usbv2_adapter_exit: fe0[0]->dvb= ffff880078cba580
usb 1-1: dvb_usbv2_adapter_exit: fe1[0]= ffff88003698e830
usb 1-1: dvb_usbv2_adapter_exit: fe1[0]->dvb= 003a746165733a3d
usb 1-1: dvb_usbv2_adapter_frontend_exit: adap=0
usb 1-1: dvb_usbv2_adapter_frontend_exit: fe[1]= ffff88003698e830
usb 1-1: dvb_usbv2_adapter_frontend_exit: fe[1]->dvb= 003a746165733a3d

Later on in dvb_usbv2_adapter_frontend_exit() fe[1]->dvb is dereferenced 
and thus causes a kernel crash.

So for some reason fe[1]->dvb gets corrupted. It doesn't happen all the 
time but after max 3 times I get this crash. I have reproduced this on 
my main machine running Ubuntu 14.04, 14.10 and a VM running Ubuntu 
14.04 all running stock kernel (3.13 and 3.16) and the media_build back 
port code.

After some investigation I saw that fe[1]->demodulator_priv also gets 
corrupted. Something is overwriting the pointers.

So with that knowledge I wrote the following patch and now I can freely 
reload the driver without a crash. This of course doesn't fix the issue 
but just corrupts unused dummy memory.

So does anyone have any hunch on what might be causing this issue or how 
to track it down ?
Keep in mind that this could be caused by me running the media_build 
code or some bug in the driver. Or it could also affect the regular tree 
when unplugging devices with more then 1 frontend.

MvH
Benjamin Larsson


diff --git a/drivers/media/dvb-core/dvb_frontend.h 
b/drivers/media/dvb-core/dvb_frontend.h
index 816269e..e0ba434 100644
--- a/drivers/media/dvb-core/dvb_frontend.h
+++ b/drivers/media/dvb-core/dvb_frontend.h
@@ -413,19 +413,30 @@ struct dtv_frontend_properties {
  #define DVB_FE_DEVICE_RESUME    3

  struct dvb_frontend {
-       struct dvb_frontend_ops ops;
-       struct dvb_adapter *dvb;
         void *demodulator_priv;
+       int dummy1[16000];
         void *tuner_priv;
+       int dummy2[16000];
         void *frontend_priv;
+       int dummy3[16000];
         void *sec_priv;
+       int dummy4[16000];
         void *analog_demod_priv;
+       int dummy5[16000];
         struct dtv_frontend_properties dtv_property_cache;
+       int dummy6[16000];
  #define DVB_FRONTEND_COMPONENT_TUNER 0
  #define DVB_FRONTEND_COMPONENT_DEMOD 1
         int (*callback)(void *adapter_priv, int component, int cmd, int 
arg);
+       int dummy7[16000];
         int id;
+       int dummy8[16000];
         unsigned int exit;
+       int dummy9[16000];
+       struct dvb_frontend_ops ops;
+       int dummy10[16000];
+       struct dvb_adapter *dvb;
+       int dummy11[16000];
  };

Re: Random memory corruption of fe[1]->dvb pointer

[Benjamin Larsson]

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:

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);


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

Re: Random memory corruption of fe[1]->dvb pointer

[Akihiro TSUKADA]

> 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

Re: Random memory corruption of fe[1]->dvb pointer

[Antti Palosaari]

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

-- 
http://palosaari.fi/

Re: Random memory corruption of fe[1]->dvb pointer

[Benjamin Larsson]

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

Re: Random memory corruption of fe[1]->dvb pointer

[Antti Palosaari]

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

-- 
http://palosaari.fi/

Re: Random memory corruption of fe[1]->dvb pointer

[Benjamin Larsson]

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