netdev - Re: [PATCH 4/4] bgmac: fix DMA rx corruption

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 12 Apr 2015 12:43:56 +0200
From:	Felix Fietkau <nbd@...nwrt.org>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	netdev@...r.kernel.org, zajec5@...il.com, hauke@...ke-m.de
Subject: Re: [PATCH 4/4] bgmac: fix DMA rx corruption

On 2015-04-12 12:31, Eric Dumazet wrote:
> On Sun, 2015-04-12 at 12:08 +0200, Felix Fietkau wrote:
>> The driver needs to inform the hardware about the first invalid (not yet
>> filled) rx slot, by writing its DMA descriptor pointer offset to the
>> BGMAC_DMA_RX_INDEX register.
>> 
>> This register was set to a value exceeding the rx ring size, effectively
>> allowing the hardware constant access to the full ring, regardless of
>> which slots are initialized.
>> 
>> Fix this by updating the register in bgmac_dma_rx_setup_desc.
>> 
>> Signed-off-by: Felix Fietkau <nbd@...nwrt.org>
>> ---
>>  drivers/net/ethernet/broadcom/bgmac.c | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
>> index e332de8..856ceee 100644
>> --- a/drivers/net/ethernet/broadcom/bgmac.c
>> +++ b/drivers/net/ethernet/broadcom/bgmac.c
>> @@ -693,10 +697,6 @@ static void bgmac_dma_init(struct bgmac *bgmac)
>>  		for (j = 0; j < ring->num_slots; j++)
>>  			bgmac_dma_rx_setup_desc(bgmac, ring, j);
>>  
> 
> Missing dma_wmb() here  (or legacy wmb() for stable kernels)
Right, thanks.

>> -		bgmac_write(bgmac, ring->mmio_base + BGMAC_DMA_RX_INDEX,
>> -			    ring->index_base +
>> -			    ring->num_slots * sizeof(struct bgmac_dma_desc));
>> -
> 
> 
> This might be better for performance to perform one single bgmac_write()
> at the end of bgmac_dma_rx_read(), and leave this one in place as well,
> not for performance since this is slow path, but correctness.
I intentionally made it write this field for every slot update, because
it might potentially allow the hardware to push frames faster when under
pressure. The CPU isn't fast enough to handle gigabit speeds anyway.

And by the way, the value that is written in this removed call is quite
wrong, so I'd rather just remove it instead of leaving a duplicate here.

> Also I am surprised there is no memory barrier to make sure changes to
> dma_desc are committed to memory ?
> 
> I would use dma_wmb() before bgmac_write(), like the wmb() done in
> bgmac_dma_tx_add()
Will add that.

- Felix
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html