netdev - Re: [PATCH 0/2] vmxnet3: Fix inconsistent DMA accesses

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: <CAOZ5it3cgGB6D8jsFp2oRCY5DpO5hoomsi-OvP+55R2cfwkGgA@mail.gmail.com>
Date: Mon, 18 Nov 2024 08:31:35 -0700
From: Brian Johannesmeyer <bjohannesmeyer@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Ronak Doshi <ronak.doshi@...adcom.com>, 
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>, Andrew Lunn <andrew+netdev@...n.ch>, 
	"David S . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Paolo Abeni <pabeni@...hat.com>, Andy King <acking@...are.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Raphael Isemann <teemperor@...il.com>
Subject: Re: [PATCH 0/2] vmxnet3: Fix inconsistent DMA accesses

> But committing patch 1 just
> to completely revert it in patch 2 seems a little odd.

Indeed, this was a poor choice on my part. I suppose the correct way
to do this would be to submit them separately (as opposed to as a
series)? I.e.: (i) one patch to start adding the synchronization
operations (in case `adapter` should indeed be in a DMA region), and
(ii) a second patch to remove `adapter` from a DMA region? Based on
the feedback, I can submit a V2 patch for either (i) or (ii).

> Also trivial note, please checkpatch with --strict --max-line-length=80

Thanks for the feedback.

-Brian

On Thu, Nov 14, 2024 at 8:38 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Wed, 13 Nov 2024 20:59:59 +0100 Brian Johannesmeyer wrote:
> > We found hundreds of inconsistent DMA accesses in the VMXNET3 driver. This
> > patch series aims to fix them. (For a nice summary of the rules around
> > accessing streaming DMA --- which, if violated, result in inconsistent
> > accesses --- see Figure 4a of this paper [0]).
> >
> > The inconsistent accesses occur because the `adapter` object is mapped into
> > streaming DMA. However, when it is mapped into streaming DMA, it is then
> > "owned" by the device. Hence, any access to `adapter` thereafter, if not
> > preceded by a CPU-synchronization operation (e.g.,
> > `dma_sync_single_for_cpu()`), may cause unexpected hardware behaviors.
> >
> > This patch series consists of two patches:
> > - Patch 1 adds synchronization operations into `vmxnet3_probe_device()`, to
> >   mitigate the inconsistent accesses when `adapter` is initialized.
> > However, this unfortunately does not mitigate all inconsistent accesses to
> > it, because `adapter` is accessed elsewhere in the driver without proper
> > synchronization.
> > - Patch 2 removes `adapter` from streaming DMA, which entirely mitigates
> >   the inconsistent accesses to it. It is not clear to me why `adapter` was
> > mapped into DMA in the first place (in [1]), because it seems that before
> > [1], it was not mapped into DMA. (However, I am not very familiar with the
> > VMXNET3 internals, so someone is welcome to correct me here). Alternatively
> > --- if `adapter` should indeed remain mapped in DMA --- then
> > synchronization operations should be added throughout the driver code (as
> > Patch 1 begins to do).
>
> I guess we need to hear from vmxnet3 maintainers to know whether DMA
> mapping is necessary for this virt device. But committing patch 1 just
> to completely revert it in patch 2 seems a little odd.
>
> Also trivial note, please checkpatch with --strict --max-line-length=80
> --
> pw-bot: cr