Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 12 Jul 2021 13:36:19 -0400
From:   Zekun Shen <bruceshenzk@gmail.com>
To:     Igor Russkikh <irusskikh@marvell.com>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [EXT] [PATCH] [net][atlantic] Fix buff_ring OOB in
 aq_ring_rx_clean
Message-ID: <YOx9k4vDcerEEbWn@Zekuns-MBP-16.fios-router.home>
References: <YOtc3GEEVonOb1lf@Zekuns-MBP-16.fios-router.home>
 <6ded0985-7707-4bde-adb2-ee1f411055d7@marvell.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6ded0985-7707-4bde-adb2-ee1f411055d7@marvell.com>
Precedence: bulk

On Mon, Jul 12, 2021 at 06:33:37PM +0200, Igor Russkikh wrote:
> From code analysis, the only way how ->next could be overflowed - is a
> hardware malfunction/data corruption.
Yes. The unchecked index field is within a buffer ring, which I assume is a DMA region.
A faulty or compromised hardware could trigger the OOB bug. Leaving it undetected could
cause memory corruption, so the patch returns with I/O error.

> Software driver logic can't lead to that field overflow.
> I'm not sure how fuzzing can lead to that result.. Do you have any details?
The fuzzer we used is targeting the hardware input vector including MMIO and DMA.

> Even if it can, then we should also do a similar check in `if (buff->is_eop)` case below,
> since it also uses similar sequence to run through `next` pointers.
Thanks for pointing out. That should be checked too.