On Thu, Jun 21, 2018 at 07:46:12PM +0200, Sebastien Marie wrote:
> Hi,
>
> m...@netbsd.org has corrected an use-after-free on NetBSD on similar
> code we have.
>
> Fix use-after-free, m_cat can free m.
>
> http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/net80211/ieee80211_input.c.diff?r1=1.111&r2=1.112
>
>
> >From code reading on us side, I think the same problem is present.
Yes, it is. More below.
> net80211/ieee80211_input.c
> 538 /*
> 539 * Handle defragmentation (see 9.5 and Annex C). We support the
> concurrent
> 540 * reception of fragments of three fragmented MSDUs or MMPDUs.
> 541 */
> 542 struct mbuf *
> 543 ieee80211_defrag(struct ieee80211com *ic, struct mbuf *m, int hdrlen)
> 544 {
> ...
> 597 /* strip 802.11 header and concatenate fragment */
> 598 m_adj(m, hdrlen);
> 599 m_cat(df->df_m, m);
> 600 df->df_m->m_pkthdr.len += m->m_pkthdr.len;
>
>
> the second argument of m_cat() `m' could be freed: m_cat() could call
> m_free() on it. so I assume it isn't safe to deference it to get
> `m_pkthdr.len`.
>
> Here m_cat() code:
>
> kern/uipc_mbuf.c
> 772 /*
> 773 * Concatenate mbuf chain n to m.
> 774 * n might be copied into m (when n->m_len is small), therefore data
> portion of
> 775 * n could be copied into an mbuf of different mbuf type.
> 776 * Therefore both chains should be of the same type (e.g. MT_DATA).
> 777 * Any m_pkthdr is not updated.
> 778 */
> 779 void
> 780 m_cat(struct mbuf *m, struct mbuf *n)
> 781 {
> 782 while (m->m_next)
> 783 m = m->m_next;
> 784 while (n) {
> 785 if (M_READONLY(m) || n->m_len > M_TRAILINGSPACE(m)) {
> 786 /* just join the two chains */
> 787 m->m_next = n;
> 788 return;
> 789 }
> 790 /* splat the data from one into the other */
> 791 memcpy(mtod(m, caddr_t) + m->m_len, mtod(n, caddr_t),
> 792 n->m_len);
> 793 m->m_len += n->m_len;
> 794 n = m_free(n);
> 795 }
> 796 }
>
>
> the NetBSD diff seems to mix 2 things. I understand the first (saving
> `m->m_pkthdr.len' in intermediate variable for adjust
> `df->df_m->m_pkthdr.len' later), but I am unsure to the second.
What the second part is about is that 'wh' equals 'm->m_data'.
There is another possibility for use-after-free by dereferencing
'wh' after 'm' was freed in case m->m_data equals m_ext.ext_buf.
For wireless frames in the Rx path this is very likely to be
the case, since e.g. iwm_rx_addbuf() requires M_EXT.
m_ext.ext_buf is freed in m_extfree(m) via m_free(m).
So I think we need to account for use-after-free via 'wh', too.
> The diff below contains only the mlen part.
> Index: net80211/ieee80211_input.c
> ===================================================================
> RCS file: /cvs/src/sys/net80211/ieee80211_input.c,v
> retrieving revision 1.201
> diff -u -p -r1.201 ieee80211_input.c
> --- net80211/ieee80211_input.c 5 May 2018 06:58:05 -0000 1.201
> +++ net80211/ieee80211_input.c 21 Jun 2018 17:37:41 -0000
> @@ -546,7 +546,7 @@ ieee80211_defrag(struct ieee80211com *ic
> struct ieee80211_defrag *df;
> u_int16_t rxseq, seq;
> u_int8_t frag;
> - int i;
> + int i, mlen;
>
> wh = mtod(m, struct ieee80211_frame *);
'wh' is assigned the value of 'm->m_data' here.
> rxseq = letoh16(*(const u_int16_t *)wh->i_seq);
> @@ -596,8 +596,9 @@ ieee80211_defrag(struct ieee80211com *ic
> df->df_frag = frag;
> /* strip 802.11 header and concatenate fragment */
> m_adj(m, hdrlen);
> + mlen = m->m_pkthdr.len;
> m_cat(df->df_m, m);
> - df->df_m->m_pkthdr.len += m->m_pkthdr.len;
> + df->df_m->m_pkthdr.len += mlen;
>
I think you should also do something about deferencing 'wh'
after m_cat() on this line:
> if (wh->i_fc[1] & IEEE80211_FC1_MORE_FRAG)
> return NULL; /* MSDU or MMPDU not yet complete */
>
