On 26 August 2016 at 17:39, Daniil Egranov daniil.egranov@arm.com wrote:
Hi Alan,
On 08/25/2016 05:39 PM, Alan Ott wrote:
Explicitly zero allocated memory for DMA receive buffers to help guard against security issues.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk
Drivers/Net/MarvellYukonDxe/if_msk.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 573dea5..9bd0d12 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -585,6 +585,7 @@ msk_newbuf ( if (EFI_ERROR (Status)) { return Status; }
- ZeroMem (Buffer, Length);
Please use gBS->SetMem() instead of ZeroMem(). It's needed for EBC code optimization.
Status = mPciIo->Map (mPciIo, EfiPciIoOperationBusMasterWrite,Buffer, &Length, &PhysAddr, &Mapping); if (EFI_ERROR (Status)) {
What I still don't like about this code (but this is not a comment against /this/ patch, just something I spotted due to the patch's context) is that we are using PciIo->AllocateBuffer for streaming DMA (i.e., EfiPciIoOperationBusMasterWrite). I think I mentioned this before, but on platforms with non-coherent DMA, this will force the DMA layer to allocate uncached memory, where a simple cache invalidate would be sufficient (since the buffer is guaranteed to be aligned to the maximum cacheline size aka the CWG, whose architectural max is 2 KB)
Anyone care to propose (and test) a patch that changes this to a simple AllocatePages()?