netdev - Re: [bug report] net/funeth: probing and netdev ops

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID:
 <DS7PR21MB3388BCDD652811D1D9B090C5D9812@DS7PR21MB3388.namprd21.prod.outlook.com>
Date: Fri, 16 Aug 2024 09:36:18 +0000
From: Dimitris Michailidis <dmichailidis@...rosoft.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [bug report] net/funeth: probing and netdev ops

On Thu, 15 Aug 2024 14:29:38 +0300 Dan Carpenter wrote:
> Hello Dimitris Michailidis,
>
> Commit ee6373ddf3a9 ("net/funeth: probing and netdev ops") from Feb
> 24, 2022 (linux-next), leads to the following Smatch static checker
> warning:
>
>        drivers/net/ethernet/fungible/funeth/funeth_main.c:475 fun_free_rings()
>        warn: 'rxqs' was already freed. (line 472)
>
> drivers/net/ethernet/fungible/funeth/funeth_main.c
>     441 static void fun_free_rings(struct net_device *netdev, struct fun_qset *qset)
>     442 {
...
>     468         free_rxqs(rxqs, qset->nrxqs, qset->rxq_start, qset->state);
>     469         free_txqs(qset->txqs, qset->ntxqs, qset->txq_start, qset->state);
>     470         free_xdpqs(xdpqs, qset->nxdpqs, qset->xdpq_start, qset->state);
>     471         if (qset->state == FUN_QSTATE_DESTROYED)
>     472                 kfree(rxqs);
>                         ^^^^^^^^^^^
> Freed.
>
>     473
>     474         /* Tell the caller which queues were operated on. */
> --> 475         qset->rxqs = rxqs;
>                              ^^^^^
> why are we saving a freed pointer?

The field may be NULL on entry to the function. The assignment tells the
caller that queues were freed as this function doesn't use some other
success indicator.

Note that if the caller passes a non-NULL value to begin with, this
assignment is no-op and the field still points to freed memory, it
retains the value it had upon entry. So, the assignment doesn't make
the field more dangerous than it can be without the assignment.

Though the value is the address of freed memory, it can still be compared
to other values. It's just illegal to dereference it.

>     476         qset->xdpqs = xdpqs;
>     477 }
>
> regards,
> dan carpenter