linux-kernel - RE: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a warning message

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: <BN9PR12MB50687C00EDAFC2B1A058E1E3D3C9A@BN9PR12MB5068.namprd12.prod.outlook.com>
Date:   Fri, 6 Oct 2023 15:50:42 +0000
From:   Liming Sun <limings@...dia.com>
To:     Hans de Goede <hdegoede@...hat.com>,
        Vadim Pasternak <vadimp@...dia.com>,
        David Thompson <davthompson@...dia.com>,
        Mark Gross <markgross@...nel.org>,
        Dan Carpenter <dan.carpenter@...aro.org>
CC:     "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a warning
 message

Thanks Hans.

Below is the logic:

IS_VRING_DROP() is ONLY set to TRUE for Rx, which is done in two places:
Line 696:  *desc = &vring->drop_desc;
Line 742: desc = &vring->drop_desc;

So line 634 below will never happen when IS_VRING_DROP() is TRUE due the checking of line 633.
633         if (!is_rx)
 634                 writeq(data, fifo->tx.data);

Please correct me if it's my misunderstanding.

Thanks,
Liming

> -----Original Message-----
> From: Hans de Goede <hdegoede@...hat.com>
> Sent: Friday, October 6, 2023 8:54 AM
> To: Liming Sun <limings@...dia.com>; Vadim Pasternak
> <vadimp@...dia.com>; David Thompson <davthompson@...dia.com>; Mark
> Gross <markgross@...nel.org>; Dan Carpenter <dan.carpenter@...aro.org>
> Cc: platform-driver-x86@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a warning
> message
> 
> Hi Liming,
> 
> On 10/5/23 14:18, Liming Sun wrote:
> > This commit fixes the smatch static checker warning in
> > mlxbf_tmfifo_rxtx_word() which complains data not initialized at
> > line 634 when IS_VRING_DROP() is TRUE. This is not a real bug since
> > line 634 is for Tx while IS_VRING_DROP() is only set for Rx. So there
> > is no case that line 634 is executed when IS_VRING_DROP() is TRUE.
> >
> > This commit initializes the local data variable to avoid unnecessary
> > confusion to those static analyzing tools.
> >
> > Signed-off-by: Liming Sun <limings@...dia.com>
> > ---
> >  drivers/platform/mellanox/mlxbf-tmfifo.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c
> b/drivers/platform/mellanox/mlxbf-tmfifo.c
> > index f3696a54a2bd..ccc4b51d3379 100644
> > --- a/drivers/platform/mellanox/mlxbf-tmfifo.c
> > +++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
> > @@ -595,8 +595,8 @@ static void mlxbf_tmfifo_rxtx_word(struct
> mlxbf_tmfifo_vring *vring,
> >  {
> >  	struct virtio_device *vdev = vring->vq->vdev;
> >  	struct mlxbf_tmfifo *fifo = vring->fifo;
> > +	u64 data = 0;
> >  	void *addr;
> > -	u64 data;
> >
> >  	/* Get the buffer address of this desc. */
> >  	addr = phys_to_virt(virtio64_to_cpu(vdev, desc->addr));
> 
> 
> This will fix the warning but not the issue at hand. As Dan pointed
> out in his original bug report, the issue is that after:
> 
> 78034cbece79 ("platform/mellanox: mlxbf-tmfifo: Drop the Rx packet if no
> descriptors")
> 
> We now have this IS_VRING_DROP() check in the path, which despite
> the subject writeq(data, fifo->tx.data);is currently being applied to both rx and
> tx vring-s
> and when this returns true the memcpy from the ring to &data
> will not happen, but the code will still do:
> 
> writeq(data, fifo->tx.data);
> 
> So you may have silenced the warning now, but you will still write
> data not coming from the vring to transmit. The only difference
> is you are now guaranteed to write all zeroes.
> 
> Note another older issue is that if you hit the not enough space
> path:
> 
>        } else {
>                 /* Leftover bytes. */
>                 if (!IS_VRING_DROP(vring)) {
>                         if (is_rx)
>                                 memcpy(addr + vring->cur_len, &data,
>                                        len - vring->cur_len);
>                         else
>                                 memcpy(&data, addr + vring->cur_len,
>                                        len - vring->cur_len);
>                 }
>                 vring->cur_len = len;
>         }
> 
> Then even if IS_VRING_DROP() returns true you are only initializing some bytes
> of the 8 bytes data variable and the other bytes will stay at whatever random
> value they had before and you end up writing this random bytes when doing:
> 
> writeq(data, fifo->tx.data);
> 
> Regards,
> 
> Hans
> 
> 
>