On Thu, Sep 13, 2018 at 08:44:05AM -0400, Xavier Guerin wrote:
> >Synopsis: Kernel crash in IWM driver after resume from sleep
> >Category: kernel
> >Environment:
> System : OpenBSD 6.4
> Details : OpenBSD 6.4-beta (GENERIC.MP) #292: Mon Sep 10
> 18:26:22 MDT 2018
> dera...@amd64.openbsd.org:/usr/src/sys/arch/am
> d64/compile/GENERIC.MP
>
> Architecture: OpenBSD.amd64
> Machine : amd64
> >Description:
> IWM is connected to a 802.11G network then the machine is put
> to sleep.
> After several hours, when the machine resumes it ends up in DDB
> after maybe
> 5 minutes crashing in the IWM driver.
> >How-To-Repeat:
> 1. Connect the IWM device to a network.
> 2. Put the machine to sleep.
> 3. Wait for some hours.
> 4. Resume.
> >Fix:
> Disconnect from WiFi before sleeping.
If 5 minutes have passed since resume it seems unlikely that a
suspend/resume cycle would have anything to do with this crash.
Note that suspend/resume completely powers down the device and
will cause a new association to be created with an access point.
The post-resume state is as good as new, just like after a reboot.
The DDB trace looks more like the driver was reading or writing
beyond buffers when the firmware received some particular frame.
Can you try this diff? It makes the input length checks in
this driver a bit more precise and careful.
Index: if_iwm.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.231
diff -u -p -r1.231 if_iwm.c
--- if_iwm.c 13 Aug 2018 15:05:31 -0000 1.231
+++ if_iwm.c 13 Sep 2018 16:45:25 -0000
@@ -7111,7 +7111,7 @@ iwm_rx_mpdu(struct iwm_softc *sc, struct
struct ifnet *ifp = IC2IFP(ic);
struct iwm_rx_packet *pkt;
struct iwm_rx_mpdu_res_start *rx_res;
- uint32_t len;
+ uint16_t len;
uint32_t rx_pkt_status;
int rxfail;
@@ -7124,14 +7124,16 @@ iwm_rx_mpdu(struct iwm_softc *sc, struct
m_freem(m);
return;
}
- if (len > maxlen) {
+ if (len + sizeof(*rx_res) + sizeof(rx_pkt_status) > maxlen ||
+ len > IEEE80211_MAX_LEN) {
IC2IFP(ic)->if_ierrors++;
m_freem(m);
return;
}
- rx_pkt_status = le32toh(*(uint32_t *)(pkt->data +
- sizeof(*rx_res) + len));
+ memcpy(&rx_pkt_status, pkt->data + sizeof(*rx_res) + len,
+ sizeof(rx_pkt_status));
+ rx_pkt_status = le32toh(rx_pkt_status);
rxfail = ((rx_pkt_status & IWM_RX_MPDU_RES_STATUS_CRC_OK) == 0 ||
(rx_pkt_status & IWM_RX_MPDU_RES_STATUS_OVERRUN_OK) == 0);
if (rxfail) {
@@ -7156,7 +7158,7 @@ iwm_rx_pkt(struct iwm_softc *sc, struct
struct iwm_rx_packet *pkt;
uint32_t offset = 0, nmpdu = 0, len;
struct mbuf *m0;
- const uint32_t minsz = sizeof(uint32_t) + sizeof(struct iwm_cmd_header);
+ const size_t minsz = sizeof(pkt->len_n_flags) + sizeof(pkt->hdr);
int qid, idx, code;
bus_dmamap_sync(sc->sc_dmat, data->map, 0, IWM_RBUF_SIZE,
@@ -7175,11 +7177,10 @@ iwm_rx_pkt(struct iwm_softc *sc, struct
break;
}
- len = le32toh(pkt->len_n_flags) & IWM_FH_RSCSR_FRAME_SIZE_MSK;
- if (len < sizeof(struct iwm_cmd_header) ||
- len > (IWM_RBUF_SIZE - offset))
+ len = iwm_rx_packet_len(pkt);
+ if (len < sizeof(pkt->hdr) ||
+ len > (IWM_RBUF_SIZE - offset - minsz))
break;
- len += sizeof(uint32_t); /* account for status word */
if (code == IWM_REPLY_RX_MPDU_CMD && ++nmpdu == 1) {
/* Take mbuf m0 off the RX ring. */
@@ -7207,7 +7208,7 @@ iwm_rx_pkt(struct iwm_softc *sc, struct
}
m_adj(m, offset);
- iwm_rx_mpdu(sc, m, IWM_RBUF_SIZE - offset);
+ iwm_rx_mpdu(sc, m, IWM_RBUF_SIZE - offset - minsz);
break;
}
@@ -7452,6 +7453,7 @@ iwm_rx_pkt(struct iwm_softc *sc, struct
iwm_cmd_done(sc, pkt);
}
+ len += sizeof(pkt->len_n_flags);
offset += roundup(len, IWM_FH_RSCSR_FRAME_ALIGN);
}
