Re: [PATCH 3.2 31/34] rtl8192ce: Fix null dereference in watchdog

[Larry Finger]

On 05/16/2014 10:38 AM, Ben Hutchings wrote:
> On Fri, 2014-05-16 at 09:20 -0500, Larry Finger wrote:
> > On 05/16/2014 07:47 AM, Ben Hutchings wrote:
> > > 3.2.59-rc1 review patch.  If anyone has any objections, please let me know.
> > >
> > > ------------------
> > >
> > > From: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> > >
> > > Dmitry Semyonov reported that after upgrading from 3.2.54 to
> > > 3.2.57 the rtl8192ce driver will crash when its interface is brought
> > > up.  The oops message shows:
> > >
> > > [ 1833.611397] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
> > > [ 1833.611455] IP: [<ffffffffa0410c6a>] rtl92ce_update_hal_rate_tbl+0x29/0x4db [rtl8192ce]
> > > ...
> > > [ 1833.613326] Call Trace:
> > > [ 1833.613346]  [<ffffffffa02ad9c6>] ? rtl92c_dm_watchdog+0xd0b/0xec9 [rtl8192c_common]
> > > [ 1833.613391]  [<ffffffff8105b5cf>] ? process_one_work+0x161/0x269
> > > [ 1833.613425]  [<ffffffff8105c598>] ? worker_thread+0xc2/0x145
> > > [ 1833.613458]  [<ffffffff8105c4d6>] ? manage_workers.isra.25+0x15b/0x15b
> > > [ 1833.613496]  [<ffffffff8105f6d9>] ? kthread+0x76/0x7e
> > > [ 1833.613527]  [<ffffffff81356b74>] ? kernel_thread_helper+0x4/0x10
> > > [ 1833.613563]  [<ffffffff8105f663>] ? kthread_worker_fn+0x139/0x139
> > > [ 1833.613598]  [<ffffffff81356b70>] ? gs_change+0x13/0x13
> > >
> > > Disassembly of rtl92ce_update_hal_rate_tbl() shows that the 'sta'
> > > parameter was null.  None of the changes to the rtlwifi family between
> > > 3.2.54 and 3.2.57 seem to directly cause this, and reverting commit
> > > f78bccd79ba3 ('rtlwifi: rtl8192ce: Fix too long disable of IRQs')
> > > doesn't fix it.
> > >
> > > rtl92c_dm_watchdog() calls rtl92ce_update_hal_rate_tbl() via
> > > rtl92c_dm_refresh_rate_adaptive_mask(), which does not appear in the
> > > call trace as it was inlined.  That function has been completely
> > > removed upstream which may explain why this crash wasn't seen there.
> > >
> > > I'm not sure that it is sensible to completely remove
> > > rtl92c_dm_refresh_rate_adaptive_mask() without making other
> > > compensating changes elsewhere, so try to work around this for 3.2 by
> > > checking for a null pointer in rtl92c_dm_refresh_rate_adaptive_mask()
> > > and then skipping the call to rtl92ce_update_hal_rate_tbl().
> > >
> > > References: https://bugs.debian.org/745137
> > > References: https://bugs.debian.org/745462
> > > Reported-by: Dmitry Semyonov <linulin@xxxxxxxxx>
> > > Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> > > Cc: Larry Finger <Larry.Finger@xxxxxxxxxxxx>
> > > Cc: Chaoming Li <chaoming_li@xxxxxxxxxxxxxx>
> > > ---
> >
> > Ben,
> >
> > Your fix is a reasonable workaround. I have no explanation for this NULL pointer
> > dereference to suddenly appear; however, the pointer should have been checked
> > from the start.
>
> Are you saying this is also an upstream bug?

It is a possible upstream bug; however, crashes of the form reported by Dmitri 
have not been reported to me, but then I was not aware of his problem. A quick 
check shows that the pointers returned by ieee80211_find_sta() are checked in 
current mainline. There are some cases that need a second look. I will be 
submitting a patch to upstream and stable for any of them where the checking was 
missed.

Larry

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/