Re: use-after-free in ieee80211_defrag() [from NetBSD]
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=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.c5 May 2018 06:58:05 - 1.201
> +++ net80211/ieee80211_input.c21 Jun 2018 17:37:41 -
> @@ -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 */
>
Re: use-after-free in ieee80211_defrag() [from NetBSD]
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=1.112
>
>
> From code reading on us side, I think the same problem is present.
>
> 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;
>
If I don't mess myself, I think the use-after-free is present, but it
lives in dead code. ieee80211_defrag() function seems not be used
anywhere.
It comes from Feb 8, 2009 in a commit from damien@:
revision 1.109
date: 2009/02/08 15:34:39; author: damien; state: Exp; lines: +94 -1;
initial 802.11 defragmentation bits.
the code will allow the concurrent reception of fragments of three
fragmented MSDUs or MMPDUs as required by the 802.11 standard.
But I fail to find if it was used a day or if it is just dead code since
2009.
Whole commit:
https://github.com/openbsd/src/commit/0c2a2ba16ccde25b321c6ae91f3f5bcf12f981cf
--
Sebastien Marie
use-after-free in ieee80211_defrag() [from NetBSD]
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=1.112
>From code reading on us side, I think the same problem is present.
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.
The diff below contains only the mlen part.
I quickly checked other uses of m_cat() inside the kernel, and they
seems corrects.
Thanks.
--
Sebastien Marie
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 - 1.201
+++ net80211/ieee80211_input.c 21 Jun 2018 17:37:41 -
@@ -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 *);
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;
if (wh->i_fc[1] & IEEE80211_FC1_MORE_FRAG)
return NULL;/* MSDU or MMPDU not yet complete */
