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;
Message-ID: <6ded0985-7707-4bde-adb2-ee1f411055d7@marvell.com>
Date:   Mon, 12 Jul 2021 18:33:37 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:90.0) Gecko/20100101
 Thunderbird/90.0
Subject: Re: [EXT] [PATCH] [net][atlantic] Fix buff_ring OOB in
 aq_ring_rx_clean
Content-Language: en-US
To:     Zekun Shen <bruceshenzk@gmail.com>
CC:     "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <YOtc3GEEVonOb1lf@Zekuns-MBP-16.fios-router.home>
From:   Igor Russkikh <irusskikh@marvell.com>
In-Reply-To: <YOtc3GEEVonOb1lf@Zekuns-MBP-16.fios-router.home>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Zekun,

Thanks for looking into that.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index 24122ccda614..f915b4885831 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -365,6 +365,10 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
>  		if (!buff->is_eop) {
>  			buff_ = buff;
>  			do {
> +				if (buff_->next >= self->size) {
> +					err = -EIO;
> +					goto err_exit;
> +				}
>  				next_ = buff_->next,
>  				buff_ = &self->buff_ring[next_];
>  				is_rsc_completed =
> 

From code analysis, the only way how ->next could be overflowed - is a
hardware malfunction/data corruption.

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?

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