linux-kernel - Re: [PATCH 10/26] bus: mhi: host: use array

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aaa1bf4b-c546-402f-8ad2-667fd35caa74@kadam.mountain>
Date:   Mon, 26 Jun 2023 14:46:35 +0300
From:   Dan Carpenter <dan.carpenter@...aro.org>
To:     Jeffrey Hugo <quic_jhugo@...cinc.com>
Cc:     Julia Lawall <Julia.Lawall@...ia.fr>,
        Manivannan Sadhasivam <mani@...nel.org>, keescook@...omium.org,
        kernel-janitors@...r.kernel.org, mhi@...ts.linux.dev,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 10/26] bus: mhi: host: use array_size

On Fri, Jun 23, 2023 at 03:30:36PM -0600, Jeffrey Hugo wrote:
> On 6/23/2023 3:14 PM, Julia Lawall wrote:
> > Use array_size to protect against multiplication overflows.
> > 
> > The changes were done using the following Coccinelle semantic patch:
> > 
> > // <smpl>
> > @@
> >      expression E1, E2;
> >      constant C1, C2;
> >      identifier alloc = {vmalloc,vzalloc};
> > @@
> > (
> >        alloc(C1 * C2,...)
> > |
> >        alloc(
> > -           (E1) * (E2)
> > +           array_size(E1, E2)
> >        ,...)
> > )
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <Julia.Lawall@...ia.fr>
> > 
> > ---
> >   drivers/bus/mhi/host/init.c |    4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> > index f72fcb66f408..34a543a67068 100644
> > --- a/drivers/bus/mhi/host/init.c
> > +++ b/drivers/bus/mhi/host/init.c
> > @@ -759,8 +759,8 @@ static int parse_ch_cfg(struct mhi_controller *mhi_cntrl,
> >   	 * so to avoid any memory possible allocation failures, vzalloc is
> >   	 * used here
> >   	 */
> > -	mhi_cntrl->mhi_chan = vzalloc(mhi_cntrl->max_chan *
> > -				      sizeof(*mhi_cntrl->mhi_chan));
> > +	mhi_cntrl->mhi_chan = vzalloc(array_size(mhi_cntrl->max_chan,
> > +				      sizeof(*mhi_cntrl->mhi_chan)));
> >   	if (!mhi_cntrl->mhi_chan)
> >   		return -ENOMEM;
> > 
> > 
> 
> This doesn't seem like a good fix.
> 
> If we've overflowed the multiplication, I don't think we should continue,
> and the function should return an error.  array_size() is going to return
> SIZE_MAX, and it looks like it is possible that vzalloc() may be able to
> allocate that successfully in some scenarios.

Nope.  You can never allocate more that size_t because that's the
highest number that the kernel allocation functions can accept.

Obviously on 64bit size_t is unbelievably large.  If I remember right,
on 32bit you didn't used to be able to allocate more than 2GB without
doing all sorts of tricks.  And everyone deleted those tricks when 64bit
machines became super common.

regards,
dan carpenter