linux-kernel - Re: [PATCH net] ethernet: et131x: Add missing check after DMA map

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] [day] [month] [year] [list]

Message-ID: <5bc75927-398d-4b8c-98d9-7f321ed70ca4@gmail.com>
Date: Tue, 8 Jul 2025 09:13:58 +0200
From: Thomas Fourier <fourier.thomas@...il.com>
To: Simon Horman <horms@...nel.org>
Cc: Mark Einon <mark.einon@...il.com>, Andrew Lunn <andrew+netdev@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] ethernet: et131x: Add missing check after DMA map

On 07/07/2025 22:01, Simon Horman wrote:

> On Mon, Jul 07, 2025 at 11:09:49AM +0200, Thomas Fourier wrote:
>> The DMA map functions can fail and should be tested for errors.
>> If the mapping fails, unmap and return an error.
>>
>> Fixes: 38df6492eb51 ("et131x: Add PCIe gigabit ethernet driver et131x to drivers/net")
>> Signed-off-by: Thomas Fourier <fourier.thomas@...il.com>
> nits:
>
> 1) There are two spaces after "et131x:" in the subject.
>     One is enough.
>
> 2) I think you can drop "ethernet: " from the subject.
>     "et131x: " seems to be an appropriate prefix based on git history.
>
> ...
Ok, I'll make sure to fix that.
>
>> @@ -2578,6 +2593,28 @@ static int nic_send_packet(struct et131x_adapter *adapter, struct tcb *tcb)
>>   		       &adapter->regs->global.watchdog_timer);
>>   	}
>>   	return 0;
>> +
>> +unmap_out:
>> +	// Unmap everything from i-1 to 1
>> +	while (--i) {
>> +		frag--;
>> +		dma_addr = desc[frag].addr_lo;
>> +		dma_addr |= (u64)desc[frag].addr_hi << 32;
>> +		dma_unmap_page(&adapter->pdev->dev, dma_addr,
>> +			       desc[frag].len_vlan, DMA_TO_DEVICE);
>> +	}
> I'm probably missing something obvious. But it seems to me that frag is
> incremented iff a mapping is successful. So I think only the loop below is
> needed.
Yes, frag is incremented only after a successful mapping.

The first loop is to unmap the body of the packet (i >= 1) which is mapped

with skb_frag_dma_map() (and unmapped with dma_unmap_page).  The

second is to unmap the header which is either one or two mappings of

map_single.  Should I make the comments more explicit?

>
>> +
>> +unmap_first_out:
>> +	// unmap header
>> +	while (frag--) {
>> +		frag--;
> I don't think you want to decrement frag twice here.
>
>> +		dma_addr = desc[frag].addr_lo;
>> +		dma_addr |= (u64)desc[frag].addr_hi << 32;
>> +		dma_unmap_single(&adapter->pdev->dev, dma_addr,
>> +				 desc[frag].len_vlan, DMA_TO_DEVICE);
>> +	}
>> +
>> +	return -ENOMEM;
>>   }
>>   
>>   static int send_packet(struct sk_buff *skb, struct et131x_adapter *adapter)