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@xxxxxxxx>
> >
> > ---
> > 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
