netdev - Re: errors in alignment changes..

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]

Message-ID: <5488C447.5080906@cogentembedded.com>
Date:	Thu, 11 Dec 2014 01:08:07 +0300
From:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:	David Miller <davem@...emloft.net>, mitsuhiro.kimura.kc@...esas.com
CC:	netdev@...r.kernel.org
Subject: Re: errors in alignment changes..

On 12/10/2014 11:52 PM, David Miller wrote:

> I just re-reviewed this change:

> commit 4d6a949c62f123569fb355b6ec7f314b76f93735
> Author: Mitsuhiro Kimura <mitsuhiro.kimura.kc@...esas.com>
> Date:   Thu Nov 27 20:34:00 2014 +0900

>      sh_eth: Fix skb alloc size and alignment adjust rule.

> and it has serious problems.  Well, actually the code has
> always been broken in this area.
>
> +		/* The size of the buffer is a multiple of 16 bytes. */
> +		rxdesc->buffer_length = ALIGN(mdp->rx_buf_sz, 16);
> +		dma_map_single(&ndev->dev, skb->data, rxdesc->buffer_length,
> +			       DMA_FROM_DEVICE);
>   		rxdesc->addr = virt_to_phys(PTR_ALIGN(skb->data, 4));
>   		rxdesc->status = cpu_to_edmac(mdp, RD_RACT | RD_RFP);

> It doesn't make any sense to call dma_map_single() if you aren't
> even going to use the return value.  The DMA mapping created is
> the whole point of calling this function.

    Note that this call is just moved from above, not added by this patch.

> And then later we pass:

>   			dma_sync_single_for_cpu(&ndev->dev, rxdesc->addr,
> -						mdp->rx_buf_sz,
> +						ALIGN(mdp->rx_buf_sz, 16),
>   						DMA_FROM_DEVICE);

> rxdesc->addr as the "DMA address", but that must be the return value
> from dma_map_single() not what you've actually stored there which is
> virt_to_phys() run on the skb->data.

> This code must be fixed to:

> 1) Put the return value from dma_map_single() into a local variable,

    I have already requested such change in reply to the patch fixing several 
DMA errors at once.

>     and check for mapping errors.

    This was done by that patch IIRC. It just needs to be properly split and 
resubmitted.

> 2) On success put that return value into rxdesc->addr

    I guess we can just do:

		rxdesc->addr = dma_map_single(...);

WBR, Sergei

--
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