netdev - Re: [EXT] [PATCH net] qede: avoid uninitialized entries in coal

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: <CADEbmW0eiuMCxk7m27AVauahUz-xDGjRHewQT0U+x2tgo6ai-Q@mail.gmail.com>
Date:   Fri, 24 Feb 2023 09:05:08 +0100
From:   Michal Schmidt <mschmidt@...hat.com>
To:     Manish Chopra <manishc@...vell.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Ariel Elior <aelior@...vell.com>,
        Alok Prasad <palok@...vell.com>
Subject: Re: [EXT] [PATCH net] qede: avoid uninitialized entries in coal_entry array

[Sorry, I accidentally sent my reply with HTML, got rejected by the
list, so resending in plain text.]

On Fri, Feb 24, 2023 at 8:25 AM Manish Chopra <manishc@...vell.com> wrote:
>
> > -----Original Message-----
> > From: Michal Schmidt <mschmidt@...hat.com>
> > ----------------------------------------------------------------------
> > Even after commit 908d4bb7c54c ("qede: fix interrupt coalescing
> > configuration"), some entries of the coal_entry array may theoretically be
> > used uninitialized:
> >
> >  1. qede_alloc_fp_array() allocates QEDE_MAX_RSS_CNT entries for
> >     coal_entry. The initial allocation uses kcalloc, so everything is
> >     initialized.
> >  2. The user sets a small number of queues (ethtool -L).
> >     coal_entry is reallocated for the actual small number of queues.
> >  3. The user sets a bigger number of queues.
> >     coal_entry is reallocated bigger. The added entries are not
> >     necessarily initialized.
> >
> > In practice, the reallocations will actually keep using the originally allocated
> > region of memory, but we should not rely on it.
> >
> > The reallocation is unnecessary. coal_entry can always have
> > QEDE_MAX_RSS_CNT entries.
>
> Hi Michal,
>
> Reallocation is necessary here, the motivation for reallocation is commit b0ec5489c480
> ("qede: preserve per queue stats across up/down of interface"). The coalescing configuration
> set from ethtool also needs to be retained across the interface state change, with this change
> you are not going to retain anything but always initialize with default.

It is retained. edev->coal_entry is allocated the first time
qede_alloc_fp_array() executes for the net_device. And then it's never
freed until the destruction of the net_device in__qede_remove().

> IMO, reallocation will
> always try to use same memory which was originally allocated if the requirement fits into it and
> which is the case here (driver will not attempt to allocate anything extra which were originally
> allocated ever). So there should not be any uninitialized contents, either they will be zero or
> something which were configured from ethtool by the user previously.

Yes, that's what I wrote in my description ("In practice, [...]"). But
that's relying on an implementation detail. It is not a part of the realloc
contract. If you need all the contents preserved, then do not lie to the
kernel that you need only a small part of it, i.e. do not realloc to a
smaller size.

Michal

> Nacked-by: Manish Chopra <manishc@...vell.com>
>
> Thanks,
> Manish
>
> >
> > Fixes: 908d4bb7c54c ("qede: fix interrupt coalescing configuration")
> > Signed-off-by: Michal Schmidt <mschmidt@...hat.com>
> > ---
> >  drivers/net/ethernet/qlogic/qede/qede_main.c | 21 +++++++-------------
> >  1 file changed, 7 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c
> > b/drivers/net/ethernet/qlogic/qede/qede_main.c
> > index 4a3c3b5fb4a1..261f982ca40d 100644
> > --- a/drivers/net/ethernet/qlogic/qede/qede_main.c
> > +++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
> > @@ -963,7 +963,6 @@ static int qede_alloc_fp_array(struct qede_dev *edev)
> > {
> >       u8 fp_combined, fp_rx = edev->fp_num_rx;
> >       struct qede_fastpath *fp;
> > -     void *mem;
> >       int i;
> >
> >       edev->fp_array = kcalloc(QEDE_QUEUE_CNT(edev), @@ -974,20
> > +973,14 @@ static int qede_alloc_fp_array(struct qede_dev *edev)
> >       }
> >
> >       if (!edev->coal_entry) {
> > -             mem = kcalloc(QEDE_MAX_RSS_CNT(edev),
> > -                           sizeof(*edev->coal_entry), GFP_KERNEL);
> > -     } else {
> > -             mem = krealloc(edev->coal_entry,
> > -                            QEDE_QUEUE_CNT(edev) * sizeof(*edev-
> > >coal_entry),
> > -                            GFP_KERNEL);
> > -     }
> > -
> > -     if (!mem) {
> > -             DP_ERR(edev, "coalesce entry allocation failed\n");
> > -             kfree(edev->coal_entry);
> > -             goto err;
> > +             edev->coal_entry = kcalloc(QEDE_MAX_RSS_CNT(edev),
> > +                                        sizeof(*edev->coal_entry),
> > +                                        GFP_KERNEL);
> > +             if (!edev->coal_entry) {
> > +                     DP_ERR(edev, "coalesce entry allocation failed\n");
> > +                     goto err;
> > +             }
> >       }
> > -     edev->coal_entry = mem;
> >
> >       fp_combined = QEDE_QUEUE_CNT(edev) - fp_rx - edev->fp_num_tx;
> >
> > --
> > 2.39.1
>