CVE-2009-4027: Linux kernel mac80211 Remote Kernel Panic

I read about this issue from Eugene Teo’s email to oss-security mailing list. The bug was discovered by Johannes Berg and here you can see it as it was at net/mac80211/agg-rx.c of the 2.6.31 release of the Linux kernel…

void ieee80211_sta_stop_rx_ba_session(struct ieee80211_sub_if_data *sdata, u8 *ra, u16 tid,
                                        u16 initiator, u16 reason)
{
        struct ieee80211_local *local = sdata->local;
        struct sta_info *sta;

        /* stop HW Rx aggregation. ampdu_action existence
         * already verified in session init so we add the BUG_ON */
        BUG_ON(!local->ops->ampdu_action);

        rcu_read_lock();

        sta = sta_info_get(local, ra);
        if (!sta) {
                rcu_read_unlock();
                return;
        }

        __ieee80211_stop_rx_ba_session(sta, tid, initiator, reason);

        rcu_read_unlock();
}

The above function is called if the timer that is used for “AddBA” request expires. This routine retrieves the equivalent pointer from ‘sdata->local’ and performs a check using BUG_ON() to ensure that ‘local->ops->ampdu_action’ is not NULL. From include/net/mac80211.h we can read that this function pointer is used for:

* @ampdu_action: Perform a certain A-MPDU action
*      The RA/TID combination determines the destination and TID we want
*      the ampdu action to be performed for. The action is defined through
*      ieee80211_ampdu_mlme_action. Starting sequence number (@ssn)
*      is the first frame we expect to perform the action on. Notice
*      that TX/RX_STOP can pass NULL for this parameter.
*      Returns a negative error code on failure.

As Johannes Berg noticed, this callback could be set to NULL (and as we also read from the above comment) and thus, lead to the following code from include/asm-generic/bug.h:

/*
 * Don't use BUG() or BUG_ON() unless there's really no way out; one
 * example might be detecting data structure corruption in the middle
 * of an operation that can't be backed out of.  If the (sub)system
 * can somehow continue operating, perhaps with reduced functionality,
 * it's probably not BUG-worthy.
 *
 * If you're tempted to BUG(), think again:  is completely giving up
 * really the *only* solution?  There are usually better options, where
 * users don't need to reboot ASAP and can mostly shut down cleanly.
 */
#ifndef HAVE_ARCH_BUG
#define BUG() do { \
        printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
        panic("BUG!"); \
} while (0)
#endif

#ifndef HAVE_ARCH_BUG_ON
#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
#endif

Which is of course, a kernel panic that could be remotely triggered.

Off topic, trolling:
According to the OpenBSD developers this is just a reliability fix, hopefully, for the Linux people this is a remote vulnerability.

The patch was to remove this BUG_ON() call like this:

 	struct sta_info *sta;
 
-	/* stop HW Rx aggregation. ampdu_action existence
-	 * already verified in session init so we add the BUG_ON */
-	BUG_ON(!local->ops->ampdu_action);
-
 	rcu_read_lock();

The next bug, was also discovered and reported by Johannes Berg and you can find it under net/mac80211/agg-tx.c. This is definitely less important since the susceptible code is the following:

int ieee80211_stop_tx_ba_session(struct ieee80211_hw *hw,
                                 u8 *ra, u16 tid,
                                 enum ieee80211_back_parties initiator)
{
        struct ieee80211_local *local = hw_to_local(hw);
        struct sta_info *sta;
        int ret = 0;

        if (WARN_ON(!local->ops->ampdu_action))
                return -EINVAL;
  ...
}

Where the same check for ‘local->ops->ampdu_action’ callback takes place. Luckily, this is a WARN_ON() call that will only issue a printk() message because of this:

/*
 * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
 * significant issues that need prompt attention if they should ever
 * appear at runtime.  Use the versions with printk format strings
 * to provide better diagnostics.
 */
#ifndef __WARN
#ifndef __ASSEMBLY__
extern void warn_slowpath_fmt(const char *file, const int line,
                const char *fmt, ...) __attribute__((format(printf, 3, 4)));
extern void warn_slowpath_null(const char *file, const int line);
#define WANT_WARN_ON_SLOWPATH
#endif
#define __WARN()                warn_slowpath_null(__FILE__, __LINE__)
#define __WARN_printf(arg...)   warn_slowpath_fmt(__FILE__, __LINE__, arg)
#else
#define __WARN_printf(arg...)   do { printk(arg); __WARN(); } while (0)
#endif

#ifndef WARN_ON
#define WARN_ON(condition) ({                                           \
        int __ret_warn_on = !!(condition);                              \
        if (unlikely(__ret_warn_on))                                    \
                __WARN();                                               \
        unlikely(__ret_warn_on);                                        \
})
#endif

Of course, this ‘if’ clause was updated to remove that WARN_ON() call like this:

 	int ret = 0;
 
-	if (WARN_ON(!local->ops->ampdu_action))
+	if (!local->ops->ampdu_action)
 		return -EINVAL;
 
 	if (tid >= STA_TID_NUM)