From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tariq Toukan Subject: Re: [PATCH] net/mlx4_en: fix off by one in error handling Date: Thu, 15 Sep 2016 15:18:32 +0300 Message-ID: <2f37b3b3-af5d-6bf9-e972-1ddcb4535682@gmail.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Sebastian Ott Cc: Yishai Hadas , Tariq Toukan , netdev@vger.kernel.org, linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org List-Id: linux-rdma@vger.kernel.org On 14/09/2016 7:08 PM, Sebastian Ott wrote: > On Wed, 14 Sep 2016, Tariq Toukan wrote: >> On 14/09/2016 4:53 PM, Sebastian Ott wrote: >>> On Wed, 14 Sep 2016, Tariq Toukan wrote: >>>> On 14/09/2016 2:09 PM, Sebastian Ott wrote: >>>>> If an error occurs in mlx4_init_eq_table the index used in the >>>>> err_out_unmap label is one too big which results in a panic in >>>>> mlx4_free_eq. This patch fixes the index in the error path. >>>> You are right, but your change below does not cover all cases. >>>> The full solution looks like this: >>>> >>>> @@ -1260,7 +1260,7 @@ int mlx4_init_eq_table(struct mlx4_dev *dev) >>>> eq); >>>> } >>>> if (err) >>>> - goto err_out_unmap; >>>> + goto err_out_unmap_excluded; >>> In this case a call to mlx4_create_eq failed. Do you really have to call >>> mlx4_free_eq for this index again? >> We agree on this part, that's why here we should goto the _excluded_ label. >> For all other parts, we should not exclude the eq in the highest index, and >> thus we goto the _non_excluded_ label. > But that's exactly what the original patch does. If the failure is within > the for loop at index i, we do the cleanup starting at index i-1. If the > failure is after the for loop then i == dev->caps.num_comp_vectors + 1 > and we do the cleanup starting at index i == dev->caps.num_comp_vectors. > > In the latter case your patch would have an out of bounds array access. Indeed. Agreed. > Regards, > Sebastian > Reviewed-by: Tariq Toukan Thanks!