This series addresses issues in rx fragmentation path.
P Praneesh (3):
wifi: ath12k: change DMA direction while mapping reinjected packets
wifi: ath12k: fix invalid memory access while processing fragmented
packets
wifi: ath12k: fix firmware crash during reo reinject
drivers/net/wireless/ath/ath12k/dp_rx.c | 18 ++++++++----------
drivers/net/wireless/ath/ath12k/hw.c | 6 +-----
2 files changed, 9 insertions(+), 15 deletions(-)
base-commit: 0ef7606c6012f05a1f5398e3a1964c35eb9c08a4
--
2.25.1
For fragmented packets, ath12k reassembles each fragment as a normal
packet and then reinjects it into HW ring. In this case, the DMA
direction should be DMA_TO_DEVICE, not DMA_FROM_DEVICE. Otherwise,
an invalid payload may be reinjected into the HW and
subsequently delivered to the host.
Given that arbitrary memory can be allocated to the skb buffer,
knowledge about the data contained in the reinjected buffer is lacking.
Consequently, there’s a risk of private information being leaked.
Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00209-QCAHKSWPL_SILICONZ-1
Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
Co-developed-by: Baochen Qiang <[email protected]>
Signed-off-by: Baochen Qiang <[email protected]>
Signed-off-by: P Praneesh <[email protected]>
---
drivers/net/wireless/ath/ath12k/dp_rx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.c b/drivers/net/wireless/ath/ath12k/dp_rx.c
index 37205e894afe..2bfcc19d15ea 100644
--- a/drivers/net/wireless/ath/ath12k/dp_rx.c
+++ b/drivers/net/wireless/ath/ath12k/dp_rx.c
@@ -3004,7 +3004,7 @@ static int ath12k_dp_rx_h_defrag_reo_reinject(struct ath12k *ar,
buf_paddr = dma_map_single(ab->dev, defrag_skb->data,
defrag_skb->len + skb_tailroom(defrag_skb),
- DMA_FROM_DEVICE);
+ DMA_TO_DEVICE);
if (dma_mapping_error(ab->dev, buf_paddr))
return -ENOMEM;
@@ -3090,7 +3090,7 @@ static int ath12k_dp_rx_h_defrag_reo_reinject(struct ath12k *ar,
spin_unlock_bh(&dp->rx_desc_lock);
err_unmap_dma:
dma_unmap_single(ab->dev, buf_paddr, defrag_skb->len + skb_tailroom(defrag_skb),
- DMA_FROM_DEVICE);
+ DMA_TO_DEVICE);
return ret;
}
--
2.25.1
The monitor ring and the reo reinject ring share the same ring mask index.
When the driver receives an interrupt for the reo reinject ring, the
monitor ring is also processed, leading to invalid memory access. Since
monitor support is not yet enabled in ath12k, the ring mask for the monitor
ring should be removed.
Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00209-QCAHKSWPL_SILICONZ-1
Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
Signed-off-by: P Praneesh <[email protected]>
---
drivers/net/wireless/ath/ath12k/hw.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/hw.c b/drivers/net/wireless/ath/ath12k/hw.c
index 06f443216488..ffe06ebdc992 100644
--- a/drivers/net/wireless/ath/ath12k/hw.c
+++ b/drivers/net/wireless/ath/ath12k/hw.c
@@ -544,9 +544,6 @@ static const struct ath12k_hw_ring_mask ath12k_hw_ring_mask_qcn9274 = {
},
.rx_mon_dest = {
0, 0, 0,
- ATH12K_RX_MON_RING_MASK_0,
- ATH12K_RX_MON_RING_MASK_1,
- ATH12K_RX_MON_RING_MASK_2,
},
.rx = {
0, 0, 0, 0,
@@ -572,8 +569,7 @@ static const struct ath12k_hw_ring_mask ath12k_hw_ring_mask_qcn9274 = {
ATH12K_HOST2RXDMA_RING_MASK_0,
},
.tx_mon_dest = {
- ATH12K_TX_MON_RING_MASK_0,
- ATH12K_TX_MON_RING_MASK_1,
+ 0, 0, 0,
},
};
--
2.25.1
When handling fragmented packets, the ath12k driver reassembles each
fragment into a normal packet and then reinjects it into the HW ring.
However, a firmware crash occurs during this reinjection process.
The issue arises because the driver populates peer metadata in
reo_ent_ring->queue_addr_lo, while the firmware expects the physical
address obtained from the corresponding peer’s queue descriptor. Fix it
by filling peer's queue descriptor's physical address in queue_addr_lo.
Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00209-QCAHKSWPL_SILICONZ-1
Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
Signed-off-by: P Praneesh <[email protected]>
---
drivers/net/wireless/ath/ath12k/dp_rx.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.c b/drivers/net/wireless/ath/ath12k/dp_rx.c
index 2bfcc19d15ea..2adb6c7d4a42 100644
--- a/drivers/net/wireless/ath/ath12k/dp_rx.c
+++ b/drivers/net/wireless/ath/ath12k/dp_rx.c
@@ -2967,7 +2967,7 @@ static int ath12k_dp_rx_h_defrag_reo_reinject(struct ath12k *ar,
struct hal_srng *srng;
dma_addr_t link_paddr, buf_paddr;
u32 desc_bank, msdu_info, msdu_ext_info, mpdu_info;
- u32 cookie, hal_rx_desc_sz, dest_ring_info0;
+ u32 cookie, hal_rx_desc_sz, dest_ring_info0, queue_addr_hi;
int ret;
struct ath12k_rx_desc_info *desc_info;
enum hal_rx_buf_return_buf_manager idle_link_rbm = dp->idle_link_rbm;
@@ -3060,13 +3060,11 @@ static int ath12k_dp_rx_h_defrag_reo_reinject(struct ath12k *ar,
reo_ent_ring->rx_mpdu_info.peer_meta_data =
reo_dest_ring->rx_mpdu_info.peer_meta_data;
- /* Firmware expects physical address to be filled in queue_addr_lo in
- * the MLO scenario and in case of non MLO peer meta data needs to be
- * filled.
- * TODO: Need to handle for MLO scenario.
- */
- reo_ent_ring->queue_addr_lo = reo_dest_ring->rx_mpdu_info.peer_meta_data;
- reo_ent_ring->info0 = le32_encode_bits(dst_ind,
+ reo_ent_ring->queue_addr_lo = cpu_to_le32(lower_32_bits(rx_tid->paddr));
+ queue_addr_hi = upper_32_bits(rx_tid->paddr);
+ reo_ent_ring->info0 = le32_encode_bits(queue_addr_hi,
+ HAL_REO_ENTR_RING_INFO0_QUEUE_ADDR_HI) |
+ le32_encode_bits(dst_ind,
HAL_REO_ENTR_RING_INFO0_DEST_IND);
reo_ent_ring->info1 = le32_encode_bits(rx_tid->cur_sn,
--
2.25.1
On 5/20/2024 12:00 AM, P Praneesh wrote:
> For fragmented packets, ath12k reassembles each fragment as a normal
> packet and then reinjects it into HW ring. In this case, the DMA
> direction should be DMA_TO_DEVICE, not DMA_FROM_DEVICE. Otherwise,
> an invalid payload may be reinjected into the HW and
> subsequently delivered to the host.
>
> Given that arbitrary memory can be allocated to the skb buffer,
> knowledge about the data contained in the reinjected buffer is lacking.
> Consequently, there’s a risk of private information being leaked.
>
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00209-QCAHKSWPL_SILICONZ-1
>
> Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
> Co-developed-by: Baochen Qiang <[email protected]>
> Signed-off-by: Baochen Qiang <[email protected]>
> Signed-off-by: P Praneesh <[email protected]>
Acked-by: Jeff Johnson <[email protected]>
On 5/20/2024 12:00 AM, P Praneesh wrote:
> The monitor ring and the reo reinject ring share the same ring mask index.
> When the driver receives an interrupt for the reo reinject ring, the
> monitor ring is also processed, leading to invalid memory access. Since
> monitor support is not yet enabled in ath12k, the ring mask for the monitor
> ring should be removed.
>
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00209-QCAHKSWPL_SILICONZ-1
>
> Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
> Signed-off-by: P Praneesh <[email protected]>
Acked-by: Jeff Johnson <[email protected]>
On 5/20/2024 12:00 AM, P Praneesh wrote:
> When handling fragmented packets, the ath12k driver reassembles each
> fragment into a normal packet and then reinjects it into the HW ring.
> However, a firmware crash occurs during this reinjection process.
> The issue arises because the driver populates peer metadata in
> reo_ent_ring->queue_addr_lo, while the firmware expects the physical
> address obtained from the corresponding peer’s queue descriptor. Fix it
> by filling peer's queue descriptor's physical address in queue_addr_lo.
>
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00209-QCAHKSWPL_SILICONZ-1
>
> Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
> Signed-off-by: P Praneesh <[email protected]>
Acked-by: Jeff Johnson <[email protected]>
On Mon May 20, 2024 at 9:00 AM CEST, P Praneesh wrote:
> When handling fragmented packets, the ath12k driver reassembles each
> fragment into a normal packet and then reinjects it into the HW ring.
> However, a firmware crash occurs during this reinjection process.
> The issue arises because the driver populates peer metadata in
> reo_ent_ring->queue_addr_lo, while the firmware expects the physical
> address obtained from the corresponding peer’s queue descriptor. Fix it
> by filling peer's queue descriptor's physical address in queue_addr_lo.
>
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00209-QCAHKSWPL_SILICONZ-1
>
> Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
> Signed-off-by: P Praneesh <[email protected]>
> ---
> drivers/net/wireless/ath/ath12k/dp_rx.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.c b/drivers/net/wireless/ath/ath12k/dp_rx.c
> index 2bfcc19d15ea..2adb6c7d4a42 100644
> --- a/drivers/net/wireless/ath/ath12k/dp_rx.c
> +++ b/drivers/net/wireless/ath/ath12k/dp_rx.c
> @@ -2967,7 +2967,7 @@ static int ath12k_dp_rx_h_defrag_reo_reinject(struct ath12k *ar,
> struct hal_srng *srng;
> dma_addr_t link_paddr, buf_paddr;
> u32 desc_bank, msdu_info, msdu_ext_info, mpdu_info;
> - u32 cookie, hal_rx_desc_sz, dest_ring_info0;
> + u32 cookie, hal_rx_desc_sz, dest_ring_info0, queue_addr_hi;
> int ret;
> struct ath12k_rx_desc_info *desc_info;
> enum hal_rx_buf_return_buf_manager idle_link_rbm = dp->idle_link_rbm;
> @@ -3060,13 +3060,11 @@ static int ath12k_dp_rx_h_defrag_reo_reinject(struct ath12k *ar,
> reo_ent_ring->rx_mpdu_info.peer_meta_data =
> reo_dest_ring->rx_mpdu_info.peer_meta_data;
>
> - /* Firmware expects physical address to be filled in queue_addr_lo in
> - * the MLO scenario and in case of non MLO peer meta data needs to be
> - * filled.
> - * TODO: Need to handle for MLO scenario.
> - */
> - reo_ent_ring->queue_addr_lo = reo_dest_ring->rx_mpdu_info.peer_meta_data;
> - reo_ent_ring->info0 = le32_encode_bits(dst_ind,
> + reo_ent_ring->queue_addr_lo = cpu_to_le32(lower_32_bits(rx_tid->paddr));
> + queue_addr_hi = upper_32_bits(rx_tid->paddr);
Shouldn't there be a cpu_to_le32 somewhere here ? It just seems asymetrical
between the two values extracted from rx_tid->paddr
> + reo_ent_ring->info0 = le32_encode_bits(queue_addr_hi,
> + HAL_REO_ENTR_RING_INFO0_QUEUE_ADDR_HI) |
> + le32_encode_bits(dst_ind,
> HAL_REO_ENTR_RING_INFO0_DEST_IND);
>
> reo_ent_ring->info1 = le32_encode_bits(rx_tid->cur_sn,
Nicolas Escande <[email protected]> wrote:
[...]
> > - reo_ent_ring->queue_addr_lo = reo_dest_ring->rx_mpdu_info.peer_meta_data;
> > - reo_ent_ring->info0 = le32_encode_bits(dst_ind,
> > + reo_ent_ring->queue_addr_lo = cpu_to_le32(lower_32_bits(rx_tid->paddr));
> > + queue_addr_hi = upper_32_bits(rx_tid->paddr);
> Shouldn't there be a cpu_to_le32 somewhere here ? It just seems asymetrical
> between the two values extracted from rx_tid->paddr
> > + reo_ent_ring->info0 = le32_encode_bits(queue_addr_hi,
le32_encode_bits() will convert queue_addr_hi from cpu-order to le-order.
> > + HAL_REO_ENTR_RING_INFO0_QUEUE_ADDR_HI) |
> > + le32_encode_bits(dst_ind,
> > HAL_REO_ENTR_RING_INFO0_DEST_IND);
> >
> > reo_ent_ring->info1 = le32_encode_bits(rx_tid->cur_sn,
>
On 5/21/2024 2:20 PM, Nicolas Escande wrote:
> On Mon May 20, 2024 at 9:00 AM CEST, P Praneesh wrote:
>> When handling fragmented packets, the ath12k driver reassembles each
>> fragment into a normal packet and then reinjects it into the HW ring.
>> However, a firmware crash occurs during this reinjection process.
>> The issue arises because the driver populates peer metadata in
>> reo_ent_ring->queue_addr_lo, while the firmware expects the physical
>> address obtained from the corresponding peer’s queue descriptor. Fix it
>> by filling peer's queue descriptor's physical address in queue_addr_lo.
>>
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00209-QCAHKSWPL_SILICONZ-1
>>
>> Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
>> Signed-off-by: P Praneesh <[email protected]>
>> ---
>> drivers/net/wireless/ath/ath12k/dp_rx.c | 14 ++++++--------
>> 1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.c b/drivers/net/wireless/ath/ath12k/dp_rx.c
>> index 2bfcc19d15ea..2adb6c7d4a42 100644
>> --- a/drivers/net/wireless/ath/ath12k/dp_rx.c
>> +++ b/drivers/net/wireless/ath/ath12k/dp_rx.c
>> @@ -2967,7 +2967,7 @@ static int ath12k_dp_rx_h_defrag_reo_reinject(struct ath12k *ar,
>> struct hal_srng *srng;
>> dma_addr_t link_paddr, buf_paddr;
>> u32 desc_bank, msdu_info, msdu_ext_info, mpdu_info;
>> - u32 cookie, hal_rx_desc_sz, dest_ring_info0;
>> + u32 cookie, hal_rx_desc_sz, dest_ring_info0, queue_addr_hi;
>> int ret;
>> struct ath12k_rx_desc_info *desc_info;
>> enum hal_rx_buf_return_buf_manager idle_link_rbm = dp->idle_link_rbm;
>> @@ -3060,13 +3060,11 @@ static int ath12k_dp_rx_h_defrag_reo_reinject(struct ath12k *ar,
>> reo_ent_ring->rx_mpdu_info.peer_meta_data =
>> reo_dest_ring->rx_mpdu_info.peer_meta_data;
>>
>> - /* Firmware expects physical address to be filled in queue_addr_lo in
>> - * the MLO scenario and in case of non MLO peer meta data needs to be
>> - * filled.
>> - * TODO: Need to handle for MLO scenario.
>> - */
>> - reo_ent_ring->queue_addr_lo = reo_dest_ring->rx_mpdu_info.peer_meta_data;
>> - reo_ent_ring->info0 = le32_encode_bits(dst_ind,
>> + reo_ent_ring->queue_addr_lo = cpu_to_le32(lower_32_bits(rx_tid->paddr));
>> + queue_addr_hi = upper_32_bits(rx_tid->paddr);
> Shouldn't there be a cpu_to_le32 somewhere here ? It just seems asymetrical
> between the two values extracted from rx_tid->paddr
le32_encode_bits of queue_addr_hi does that conversion, so there is no
need to explicitly convert cpu_to_le32 while fetching rx_tid->paddr's
upper 32 bits.
>> + reo_ent_ring->info0 = le32_encode_bits(queue_addr_hi,
>> + HAL_REO_ENTR_RING_INFO0_QUEUE_ADDR_HI) |
>> + le32_encode_bits(dst_ind,
>> HAL_REO_ENTR_RING_INFO0_DEST_IND);
>>
>> reo_ent_ring->info1 = le32_encode_bits(rx_tid->cur_sn,
>
On Wed May 22, 2024 at 9:15 AM CEST, Praneesh P wrote:
>
>
> On 5/21/2024 2:20 PM, Nicolas Escande wrote:
> > On Mon May 20, 2024 at 9:00 AM CEST, P Praneesh wrote:
> >> When handling fragmented packets, the ath12k driver reassembles each
> >> fragment into a normal packet and then reinjects it into the HW ring.
> >> However, a firmware crash occurs during this reinjection process.
> >> The issue arises because the driver populates peer metadata in
> >> reo_ent_ring->queue_addr_lo, while the firmware expects the physical
> >> address obtained from the corresponding peer’s queue descriptor. Fix it
> >> by filling peer's queue descriptor's physical address in queue_addr_lo.
> >>
> >> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00209-QCAHKSWPL_SILICONZ-1
> >>
> >> Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
> >> Signed-off-by: P Praneesh <[email protected]>
> >> ---
> >> drivers/net/wireless/ath/ath12k/dp_rx.c | 14 ++++++--------
> >> 1 file changed, 6 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.c b/drivers/net/wireless/ath/ath12k/dp_rx.c
> >> index 2bfcc19d15ea..2adb6c7d4a42 100644
> >> --- a/drivers/net/wireless/ath/ath12k/dp_rx.c
> >> +++ b/drivers/net/wireless/ath/ath12k/dp_rx.c
> >> @@ -2967,7 +2967,7 @@ static int ath12k_dp_rx_h_defrag_reo_reinject(struct ath12k *ar,
> >> struct hal_srng *srng;
> >> dma_addr_t link_paddr, buf_paddr;
> >> u32 desc_bank, msdu_info, msdu_ext_info, mpdu_info;
> >> - u32 cookie, hal_rx_desc_sz, dest_ring_info0;
> >> + u32 cookie, hal_rx_desc_sz, dest_ring_info0, queue_addr_hi;
> >> int ret;
> >> struct ath12k_rx_desc_info *desc_info;
> >> enum hal_rx_buf_return_buf_manager idle_link_rbm = dp->idle_link_rbm;
> >> @@ -3060,13 +3060,11 @@ static int ath12k_dp_rx_h_defrag_reo_reinject(struct ath12k *ar,
> >> reo_ent_ring->rx_mpdu_info.peer_meta_data =
> >> reo_dest_ring->rx_mpdu_info.peer_meta_data;
> >>
> >> - /* Firmware expects physical address to be filled in queue_addr_lo in
> >> - * the MLO scenario and in case of non MLO peer meta data needs to be
> >> - * filled.
> >> - * TODO: Need to handle for MLO scenario.
> >> - */
> >> - reo_ent_ring->queue_addr_lo = reo_dest_ring->rx_mpdu_info.peer_meta_data;
> >> - reo_ent_ring->info0 = le32_encode_bits(dst_ind,
> >> + reo_ent_ring->queue_addr_lo = cpu_to_le32(lower_32_bits(rx_tid->paddr));
> >> + queue_addr_hi = upper_32_bits(rx_tid->paddr);
> > Shouldn't there be a cpu_to_le32 somewhere here ? It just seems asymetrical
> > between the two values extracted from rx_tid->paddr
> le32_encode_bits of queue_addr_hi does that conversion, so there is no
> need to explicitly convert cpu_to_le32 while fetching rx_tid->paddr's
> upper 32 bits.
OK, got it,
> >> + reo_ent_ring->info0 = le32_encode_bits(queue_addr_hi,
> >> + HAL_REO_ENTR_RING_INFO0_QUEUE_ADDR_HI) |
> >> + le32_encode_bits(dst_ind,
> >> HAL_REO_ENTR_RING_INFO0_DEST_IND);
> >>
> >> reo_ent_ring->info1 = le32_encode_bits(rx_tid->cur_sn,
> >
Thanks
P Praneesh <[email protected]> wrote:
> For fragmented packets, ath12k reassembles each fragment as a normal
> packet and then reinjects it into HW ring. In this case, the DMA
> direction should be DMA_TO_DEVICE, not DMA_FROM_DEVICE. Otherwise,
> an invalid payload may be reinjected into the HW and
> subsequently delivered to the host.
>
> Given that arbitrary memory can be allocated to the skb buffer,
> knowledge about the data contained in the reinjected buffer is lacking.
> Consequently, there’s a risk of private information being leaked.
>
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00209-QCAHKSWPL_SILICONZ-1
>
> Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
> Co-developed-by: Baochen Qiang <[email protected]>
> Signed-off-by: Baochen Qiang <[email protected]>
> Signed-off-by: P Praneesh <[email protected]>
> Acked-by: Jeff Johnson <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>
3 patches applied to ath-next branch of ath.git, thanks.
33322e3ef074 wifi: ath12k: change DMA direction while mapping reinjected packets
073f9f249eec wifi: ath12k: fix invalid memory access while processing fragmented packets
a57ab7cced45 wifi: ath12k: fix firmware crash during reo reinject
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches