ipc_mux_dl_acb_decode() walks the command chain of a received
Aggregated Command Block. first_cmd_index is passed through
array_index_nospec() with sizeof(struct mux_cmdh) as its bound, which
treats a byte offset as an array index: every valid offset at or beyond
that size is turned into zero, so the command it points at is dropped.
next_cmd_index is not validated at all before it is followed, so it can
point outside the received block, back at an already processed command,
or into the header just read. The walk can therefore read past the end
of the skb, revisit an earlier command indefinitely, or decode an
overlapping command header.
The overlapping case does not fault, which makes it the quiet one: the
current walk does not check that next_cmd_index advances beyond the
header just read. An index into that header is therefore followed, and
those bytes are decoded a second time as another command and
acknowledged a second time.
Bound the commands by the received length and require every command
offset to leave room for a complete command header. block_length is
checked to cover the block header and not to exceed the received
buffer, but it is not used to delimit the commands: the driver gives
the field no meaning beyond the header on this path, and its own
uplink encoder leaves block_length at the size of the block header.
What arrived holds either way.
Validate the link to the next command before the command carrying it is
processed: a non zero index has to advance past the current header, stay
inside the received data and leave room for a complete next header. Only
a zero index terminates the chain, which is how the uplink side of the
driver terminates its own command blocks as well. The offsets then
strictly increase, so the walk terminates, and a command is no longer
acted upon before its link is known to be usable. The array_index_nospec()
call is gone with these checks, which constrain the same value properly.
Fixes: 1f52d7b62285 ("net: wwan: iosm: Enable M.2 7360 WWAN card support")
Cc: <redacted>
Reviewed-by: Guanglei Zhu <redacted>
Signed-off-by: Fang Xieyan <redacted>
---
drivers/net/wwan/iosm/iosm_ipc_mux_codec.c | 46 ++++++++++++++++++----
1 file changed, 38 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c b/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c
index b573067a366f..2c4f175233a1 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c
@@ -526,30 +526,60 @@ static void ipc_mux_dl_acbcmd_decode(struct iosm_mux *ipc_mux,
/* Decode an aggregated command block. */
static void ipc_mux_dl_acb_decode(struct iosm_mux *ipc_mux, struct sk_buff *skb)
{
+ size_t hdr_len = offsetof(struct mux_cmdh, param);
struct mux_acbh *acbh;
struct mux_cmdh *cmdh;
- u32 next_cmd_index;
+ u32 block_length, cmd_index, next_cmd_index;
u8 *block;
int size;
+ if (skb->len < sizeof(struct mux_acbh))
+ return;
+
acbh = (struct mux_acbh *)(skb->data);
block = (u8 *)(skb->data);
+ block_length = le32_to_cpu(acbh->block_length);
- next_cmd_index = le32_to_cpu(acbh->first_cmd_index);
- next_cmd_index = array_index_nospec(next_cmd_index,
- sizeof(struct mux_cmdh));
+ /*
+ * Keep block_length within the received buffer, but use skb->len
+ * to bound the command chain: the uplink encoder sets
+ * block_length to the ACBH header size.
+ */
+ if (block_length < sizeof(struct mux_acbh) || block_length > skb->len)
+ return;
+
+ cmd_index = le32_to_cpu(acbh->first_cmd_index);
+
+ while (cmd_index != 0) {
+ /* Every command header has to lie inside the received data. */
+ if (cmd_index < sizeof(struct mux_acbh) ||
+ cmd_index > skb->len ||
+ hdr_len > skb->len - cmd_index)
+ break;
- while (next_cmd_index != 0) {
- cmdh = (struct mux_cmdh *)&block[next_cmd_index];
+ cmdh = (struct mux_cmdh *)&block[cmd_index];
next_cmd_index = le32_to_cpu(cmdh->next_cmd_index);
+
+ /*
+ * The next index has to move past this header, stay inside
+ * the received data and leave room for a header of its own.
+ */
+ if (next_cmd_index != 0 &&
+ (next_cmd_index <= cmd_index ||
+ next_cmd_index - cmd_index < hdr_len ||
+ next_cmd_index > skb->len ||
+ skb->len - next_cmd_index < hdr_len))
+ break;
+
if (ipc_mux_dl_cmdresps_decode_process(ipc_mux, cmdh->param,
cmdh->command_type,
cmdh->if_id,
cmdh->transaction_id)) {
- size = offsetof(struct mux_cmdh, param) +
- sizeof(cmdh->param.flow_ctl);
+ size = hdr_len + sizeof(cmdh->param.flow_ctl);
ipc_mux_dl_acbcmd_decode(ipc_mux, cmdh, size);
}
+
+ cmd_index = next_cmd_index;
}
}
--
2.50.1