[bug report] ath10k: abstract htt_rx_desc structure

[bug report] ath10k: abstract htt_rx_desc structure

[Dan Carpenter]

Hello Francesco Magliocca,

The patch 6bae9de622d3: "ath10k: abstract htt_rx_desc structure" from
Jan 12, 2022, leads to the following Smatch static checker warning:

	drivers/net/wireless/ath/ath10k/htt_rx.c:432 ath10k_htt_rx_amsdu_pop()
	warn: potential pointer math issue ('rx_desc' is a 32 bit pointer)

drivers/net/wireless/ath/ath10k/htt_rx.c
    346 static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
    347                                    struct sk_buff_head *amsdu)
    348 {
    349         struct ath10k *ar = htt->ar;
    350         struct ath10k_hw_params *hw = &ar->hw_params;
    351         int msdu_len, msdu_chaining = 0;
    352         struct sk_buff *msdu;
    353         struct htt_rx_desc *rx_desc;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

    354         struct rx_attention *rx_desc_attention;
    355         struct rx_frag_info_common *rx_desc_frag_info_common;
    356         struct rx_msdu_start_common *rx_desc_msdu_start_common;
    357         struct rx_msdu_end_common *rx_desc_msdu_end_common;
    358 

[ snip ]

    427 
    428                 last_msdu = __le32_to_cpu(rx_desc_msdu_end_common->info0) &
    429                                 RX_MSDU_END_INFO0_LAST_MSDU;
    430 
    431                 /* FIXME: why are we skipping the first part of the rx_desc? */
--> 432                 trace_ath10k_htt_rx_desc(ar, rx_desc + sizeof(u32),
                                                     ^^^^^^^^^^^^^^^^^^^^^
This is a pointer math bug.  It's possible that it should be:

	trace_ath10k_htt_rx_desc(ar, (u8 *)rx_desc + sizeof(u32),

But as your FIXME notes, it's hard to tell what's going on here...


    433                                          hw->rx_desc_ops->rx_desc_size - sizeof(u32));
    434 
    435                 if (last_msdu)
    436                         break;
    437         }

regards,
dan carpenter

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [bug report] ath10k: abstract htt_rx_desc structure

[Francesco Magliocca]

Hello Dan Carpenter,
auch, nice catch, thank you very much.
Yes, your solution is correct,
even if I think that we  could avoid skipping the first part of the
rx_desc altogether,
because it is just a trace.

Kalle, will you amend my commit or do I need to file a new patch?

Best regards,
Francesco Magliocca


Il giorno mar 1 feb 2022 alle ore 14:09 Dan Carpenter
<dan.carpenter@oracle.com> ha scritto:
>
> Hello Francesco Magliocca,
>
> The patch 6bae9de622d3: "ath10k: abstract htt_rx_desc structure" from
> Jan 12, 2022, leads to the following Smatch static checker warning:
>
>         drivers/net/wireless/ath/ath10k/htt_rx.c:432 ath10k_htt_rx_amsdu_pop()
>         warn: potential pointer math issue ('rx_desc' is a 32 bit pointer)
>
> drivers/net/wireless/ath/ath10k/htt_rx.c
>     346 static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
>     347                                    struct sk_buff_head *amsdu)
>     348 {
>     349         struct ath10k *ar = htt->ar;
>     350         struct ath10k_hw_params *hw = &ar->hw_params;
>     351         int msdu_len, msdu_chaining = 0;
>     352         struct sk_buff *msdu;
>     353         struct htt_rx_desc *rx_desc;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>     354         struct rx_attention *rx_desc_attention;
>     355         struct rx_frag_info_common *rx_desc_frag_info_common;
>     356         struct rx_msdu_start_common *rx_desc_msdu_start_common;
>     357         struct rx_msdu_end_common *rx_desc_msdu_end_common;
>     358
>
> [ snip ]
>
>     427
>     428                 last_msdu = __le32_to_cpu(rx_desc_msdu_end_common->info0) &
>     429                                 RX_MSDU_END_INFO0_LAST_MSDU;
>     430
>     431                 /* FIXME: why are we skipping the first part of the rx_desc? */
> --> 432                 trace_ath10k_htt_rx_desc(ar, rx_desc + sizeof(u32),
>                                                      ^^^^^^^^^^^^^^^^^^^^^
> This is a pointer math bug.  It's possible that it should be:
>
>         trace_ath10k_htt_rx_desc(ar, (u8 *)rx_desc + sizeof(u32),
>
> But as your FIXME notes, it's hard to tell what's going on here...
>
>
>     433                                          hw->rx_desc_ops->rx_desc_size - sizeof(u32));
>     434
>     435                 if (last_msdu)
>     436                         break;
>     437         }
>
> regards,
> dan carpenter

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [bug report] ath10k: abstract htt_rx_desc structure

[Kalle Valo]

Francesco Magliocca <franciman12@gmail.com> writes:

> Hello Dan Carpenter,
> auch, nice catch, thank you very much.
> Yes, your solution is correct,
> even if I think that we  could avoid skipping the first part of the
> rx_desc altogether,
> because it is just a trace.
>
> Kalle, will you amend my commit or do I need to file a new patch?

I don't rebase my trees, so please submit a new patch fixing this issue.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k