Received-SPF: pass (google.com: domain of linux-kernel+bounces-41588-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sun, 28 Jan 2024 11:29:33 +0100
From: Erick Archer <erick.archer@gmx.com>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: Erick Archer <erick.archer@gmx.com>,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	Jeffrey Hugo <quic_jhugo@quicinc.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	linux-arm-msm@vger.kernel.org, mhi@lists.linux.dev,
	linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] bus: mhi: ep: Use kcalloc() instead of kzalloc()
Message-ID: <20240128102933.GA2800@titan>
References: <20240120152518.13006-1-erick.archer@gmx.com>
 <43614a09-d520-4111-873a-b352bd93ea07@moroto.mountain>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <43614a09-d520-4111-873a-b352bd93ea07@moroto.mountain>
UI-OutboundReport: notjunk:1;M01:P0:Q3/bpQWk/cw=;zveX/XHwfQYoL9WZ3UioPfEM/Tp
 xxGX7AUHZW34Nn5+GSKI2ZfBTKf2YuizqDv6nVRiXN2GNmGtHWWkS/gP1ir4rdJjBVwdFOpBn
 2f2k4dODbfk1Fo9nZZ8ieql2sT8R6UDeAM0y4uio8l7y50ozF+u+PHHn86s/SD60LBra8e451
 mEWXLXg5TIjLmegrad/WA3GHwhRfE7YbVc+JBOGWUV/owimFMKhG+Jo6VHhVwPomewB/VFxfG
 kS4BKioeuOu04B7Ohv0Ucoa7GZqrQSGsUR0zEIaRKiZkJ706SNc5GcjTVNvBcdX8F7gcVXXTZ
 VwkxMx5nwq0t0fpz7i2zB0GGKh8/7kF/6Xtb9SGdypQFaNQgE+ohqzw7XlcUQIBTi25hcI4NA
 xms4Owvs0ygrwcmyE05+w88S1JJzo7bTTtYbrtyP7GYPNEsP/qKXNM+eozJI5j+dOM0q0671I
 Ju6+25h/rPgb8CKzmIEphJ5C/z5SgrjNLmHCxxJ0ZLlbRMwh4q7N6dizJ/TJlPNITz6JB5krO
 lYqeztCIwqFCNeEUCTbE5zOs/RDalgvuJpXrEkNsrZfH/dR/Z0BxFFduYOOWqVH0yMWSmK/K6
 ddWdfn9r43HsUCw2+GciMObPVdFZyvVJy+ps3XtCQYJ1MWx4xt4wOMl3MeHNB+cTqt3LP1vNj
 OkPcFnreI49ZxTHpF1nvI0/R9VnAUdmVyv4vPkWfshXREI2x3FdAe4vydVTrkDHShvgGso8SD
 pESWEy0MmV4M92JHnwcZZyQQ3FLBe6LKeQ3hXim/a2aUP4uowCQEi0YLSWssngQMl30abNzLH
 4+Ov52ORPtWYqtdpuO6n5zib60YwE1uebRMQfYU/8PD1yDHCXSsivloxaArNfaGFj856Lqs3t
 9O4lbzPWHDSm0GNFTeMC9Hi6OIxMg9x0pqXjSexC5e2eci+0xwac3kBHhstBY7+UHEXuOdSbO
 bw368zUr7Vc17rT6x2+0vQ0lTY4=
Content-Transfer-Encoding: quoted-printable

Hi Dan,

On Mon, Jan 22, 2024 at 10:15:20AM +0300, Dan Carpenter wrote:
> This code does not have an integer overflow, but it might have a
> different memory corruption bug.

I don't see this possible memory corruption bug. More info below.

> On Sat, Jan 20, 2024 at 04:25:18PM +0100, Erick Archer wrote:
> > As noted in the "Deprecated Interfaces, Language Features, Attributes,
> > and Conventions" documentation [1], size calculations (especially
> > multiplication) should not be performed in memory allocator (or simila=
r)
> > function arguments due to the risk of them overflowing. This could lea=
d
> > to values wrapping around and a smaller allocation being made than the
> > caller was expecting. Using those allocations could lead to linear
> > overflows of heap memory and other misbehaviors.
> >
> > So, use the purpose specific kcalloc() function instead of the argumen=
t
> > count * size in the kzalloc() function.
> >
>
> This one is more complicated to analyze.  I have built a Smatch cross
> function database so it's easy for me and I will help you.
>
> $ smbd.py where mhi_ep_cntrl event_rings
> drivers/pci/endpoint/functions/pci-epf-mhi.c | pci_epf_mhi_probe        =
      | (struct mhi_ep_cntrl)->event_rings | 0
> drivers/bus/mhi/ep/main.c      | mhi_ep_irq                     | (struc=
t mhi_ep_cntrl)->event_rings | min-max
> drivers/bus/mhi/ep/mmio.c      | mhi_ep_mmio_init               | (struc=
t mhi_ep_cntrl)->event_rings | 0-255
> drivers/bus/mhi/ep/mmio.c      | mhi_ep_mmio_update_ner         | (struc=
t mhi_ep_cntrl)->event_rings | 0-255
>
> The other way to figure this stuff out would be to do:
>
> $ grep -Rn "event_rings =3D " drivers/bus/mhi/ep/
> drivers/bus/mhi/ep/mmio.c:260:  mhi_cntrl->event_rings =3D FIELD_GET(MHI=
CFG_NER_MASK, regval);
> drivers/bus/mhi/ep/mmio.c:261:  mhi_cntrl->hw_event_rings =3D FIELD_GET(=
MHICFG_NHWER_MASK, regval);
> drivers/bus/mhi/ep/mmio.c:271:  mhi_cntrl->event_rings =3D FIELD_GET(MHI=
CFG_NER_MASK, regval);
> drivers/bus/mhi/ep/mmio.c:272:  mhi_cntrl->hw_event_rings =3D FIELD_GET(=
MHICFG_NHWER_MASK, regval);
>
> That means that this multiplication can never overflow so the patch
> has no effect on runtime.  The patch is still useful because we don't
> want every single person to have to do this analysis.  The kcalloc()
> function is just safer and more obviously correct.

Ok, I will send a v2 patch with more info in the commit message.

> It's a bit concerning that ->event_rings is set multiple times, but only
> allocated one time.  It's either unnecessary or there is a potential
> memory corruption bug.  If it's really necessary then there should be a
> check that the new size is <=3D the size of the original buffer that we
> allocated.

The ->event_rings is set twice. In the mhi_ep_mmio_init function and in
the mhi_ep_mmio_update_ner function.

void mhi_ep_mmio_init(struct mhi_ep_cntrl *mhi_cntrl)
{
	[...]
	mhi_cntrl->event_rings =3D FIELD_GET(MHICFG_NER_MASK, regval);
	[...]
}

void mhi_ep_mmio_update_ner(struct mhi_ep_cntrl *mhi_cntrl)
{
	[...]
	mhi_cntrl->event_rings =3D FIELD_GET(MHICFG_NER_MASK, regval);
	[...]
}

But ->event_rings does not need to be allocated because the type is a u32.

struct mhi_ep_cntrl {
	[...]
	u32 event_rings;
	[...]
};

So, I don't know what you are trying to explain to me. I'm sorry.

> I work in static analysis and I understand the struggle of trying to
> understand code to see if static checker warnings are a real bug or not.
> I'm not going to insist that you figure everything out, but I am asking
> that you at least try.  If after spending ten minutes reading the code
> you can't figure it out, then it's fine to write something like, "I
> don't know whether this multiply can really overflow or not, but let's
> make it safer by using kcalloc()."  You can put that sort of "I don't
> know information" under the --- cut off line inf you want.

Thanks a lot for the advices.

Regards,
Erick

> regards,
> dan carpenter