[PATCH linux dev-5.1] fsi: core: Fix NULL dereference issue

[Milton Miller II]

On 06/26/2019 around 02:39AM in some timezone, Lei YU wrote:

>On Wed, Jun 26, 2019 at 9:33 AM Jeremy Kerr <jk at ozlabs.org> wrote:
>>
>> Hi Lei,
>>
>> > The failure case in fsi_slave_init() is wrong and could cause
>NULL
>> > dereference issue.
>> > E.g. on FP5280G2 machine, it could get failure in
>> > fsi_slave_set_smode(),
>> > and when it does fsi rescan, kernel crashes due to:
>> >
>> >     Unable to handle kernel NULL pointer dereference at virtual
>> > address 00000060
>> >
>> > The fix is to make it not calling kfree() but just goto err_free.
>> >
>> > However, in err_free, it calls put_device() to free the device,
>it
>> > still
>> > cause issue during fsi rescan, that the device is used after
>freed.
>> >
>> >     WARNING: CPU: 0 PID: 1433 at lib/refcount.c:190
>> > refcount_sub_and_test_checked+0x94/0xac
>> >     refcount_t: underflow; use-after-free.
>> >
>> > So the put_device() is removed and "err_free" label is renamed to
>> > "fail".
>>
>> It looks like this will leak memory (through the struct fsi_slave)
>that
>> has been kzalloc()ed. After device_register, we need to call
>> put_device() to free the struct fsi_slave, but there's no mechanism
>for
>> that to happen if we remove it from fsi_slave_init().
>
>The memory is "leaked" in this function, that the slave device is not
>freed
>here. But eventually, it will be freed in fsi_slave_release() (if I
>understand
>the code correctly), so there is no leak, eventually.

This is definitely bad.  In other paths the struct device for the
fsi-slave might be kept beyond the lifetime of the fsi-master slave
device, which would lead to use after free.

Each kobject must have its own release method and be allocated via
a separate kmalloc.

>
>> The error paths for this function do need to be fixed, but I don't
>think
>> this is the right approach.
>>
>> Do you have a backtrace of the
>refcount_sub_and_test_checked+0x94/0xac
>> warning? This may not be the actual struct device that underflows.
>
>Yes, below is the full trace:

This is just one possibility.


BTW, there doesn't seem to be any MAINTAINERS entry for drivers/fsi

Milton