Re: [PATCH 2/3] net: wwan: mhi_wwan_mbim: check skb_copy_bits() return value

[Loic Poulain]

On Thu, Sep 10, 2026 at 5:34 AM Guanglei Zhu <zhugl3@xxxxxxxxxxxx> wrote:
>
> mhi_mbim_rx() ignores the return value of skb_copy_bits() when it
> copies each datagram out of the NTB.  The datagram offset and length
> come from the DPE, which is only checked to lie within the NTB
> itself, so a modem can point a datagram outside the received skb.
> The copy then fails and the freshly allocated skbn is passed to
> netif_rx() with its uninitialized contents still in place, leaking
> kernel heap memory into the network stack.
>
> Free the skb and account an error when the copy fails.
>
> Fixes: aa730a9905b7 ("net: wwan: Add MHI MBIM network driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Guanglei Zhu <zhugl3@xxxxxxxxxxxx>
> ---
>
> Verified in a QEMU guest with a fault injector pointing a DPE
> outside the received NTB: the copy fails, and the unpatched driver
> hands the uninitialized skbn to the network stack (observed as
> "unknown protocol" on bytes that were never written).  With this
> check the failed datagram is dropped and counted as an rx error.
>  drivers/net/wwan/mhi_wwan_mbim.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wwan/mhi_wwan_mbim.c b/drivers/net/wwan/mhi_wwan_mbim.c
> index ef158edeb..71e700008 100644
> --- a/drivers/net/wwan/mhi_wwan_mbim.c
> +++ b/drivers/net/wwan/mhi_wwan_mbim.c
> @@ -328,7 +328,13 @@ static void mhi_mbim_rx(struct mhi_mbim_context *mbim, struct sk_buff *skb)
>                                 continue;
>
>                         skb_put(skbn, dgram_len);
> -                       skb_copy_bits(skb, dgram_offset, skbn->data, dgram_len);
> +                       if (skb_copy_bits(skb, dgram_offset, skbn->data, dgram_len)) {
> +                               dev_kfree_skb_any(skbn);
> +                               u64_stats_update_begin(&link->rx_syncp);
> +                               u64_stats_inc(&link->rx_errors);
> +                               u64_stats_update_end(&link->rx_syncp);
> +                               continue;

We perform the same error handling in the switch cases below, so it
would be better to factor it out into a common helper or a shared
error path.

> +                       }
>
>                         switch (skbn->data[0] & 0xf0) {
>                         case 0x40:
> --
> 2.43.0
>