netdev - Re: [PATCH] net: fm10k: check size from dma region

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: <CAHE_cOtP+BD+6jyBCoADz1CsZaLgKTknE+OFHnDyDPMrwxo=wQ@mail.gmail.com>
Date:   Sat, 4 Jul 2020 18:11:26 -0400
From:   Zekun Shen <bruceshenzk@...il.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
        Netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: fm10k: check size from dma region

On Sat, Jul 04, 2020 at 12:41:07PM -0700, Alexander Duyck wrote:
> On Sat, Jul 4, 2020 at 9:37 AM Zekun Shen <bruceshenzk@...il.com> wrote:
> >
> > On Sat, Jul 04, 2020 at 09:05:48AM -0700, Alexander Duyck wrote:
> > > The upper limitation for the size should be 2K or FM10K_RX_BUFSZ, not
> > > PAGE_SIZE. Otherwise you are still capable of going out of bounds
> > > because the offset is used within the page to push the start of the
> > > region up by 2K.
> > PAGE_SIZE can drop the warning, as the dma allocated size is PAGE_SIZE.
>
> Yes, but the point I was getting at is that if you are just going to
> squelch the warning, but leave the code broken then the warning isn't
> of any use and might as well be discarded. Either you limit the value
> to 2K which is what the hardware is expected to max out at anyway, or
> you just skip the warning and assume hardware will do the right thing.
> I'm not even sure this patch is worth the effort if it is just using
> some dummy value that is still broken and simply squelches the
> warning.
>
> Could you provide more information about how you are encountering the
> error? Is this something you are seeing with an actual fm10k device,
> or is this something found via code review or static analysis?
I did not see it on a real device. I got the warning through emulation
and fuzzing, treating dma, mmio and interrupts as input vectors.
My research is on the peripheral/driver boundary.
>
> > > If this is actually fixing the warning it makes me wonder if the code
> > > performing the check is broken itself since we would still be
> > > accessing outside of the accessible DMA range.
> > The unbounded size is only passed to fm10k_add_rx_frag, which expects
> > and checks size to be less than FM10K_RX_HDR_LEN which is 256.
> >
> > In this way, any boundary between 256 and 4K should work. I could address
> > that with a second version.
>
> I was referring to the code in the DMA-API that is generating the
> warning being broken, not the code itself. If you can tell me how you
> are getting to the warning it would be useful.
>
> Anything over FM10K_RX_BUFSZ will break things. I think that is what
> you are missing. The driver splits a single 4K page into 2 pieces and
> then gives half off to the stack and uses the other half for the next
> receive. If you have a value over 2K you are going to be overwritting
> data in another buffer and/or attempting to access memory outside the
> DMA region. Both of which would likely cause significant issues and
> likely panic the system.
I agree. FM10K_RX_BUFSZ is the right boundary in that sense.