Thread (21 messages) 21 messages, 4 authors, 2021-08-18

Re: [PATCH v7 01/11] libata: fix ata_host_alloc_pinfo()

From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: 2021-08-16 11:29:42
Also in: linux-ide

On Mon, 2021-08-16 at 10:44 +0900, Damien Le Moal wrote:
quoted hunk ↗ jump to hunk
Avoid static checkers warnings about a potential NULL pointer
dereference for the port info variable pi. To do so, test that at
least
one port info is available on entry to ata_host_alloc_pinfo() and
start
the ata port initialization for() loop with pi initialized to the
first
port info passed as argument (which is already checked to be non
NULL).
Within the for() loop, get the next port info, if it is not NULL,
after initializing the ata port using the previous port info.

Reported-by: kernel test robot <redacted>
Signed-off-by: Damien Le Moal <redacted>
---
 drivers/ata/libata-core.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 61c762961ca8..b237a718ea0f 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5441,16 +5441,17 @@ struct ata_host *ata_host_alloc_pinfo(struct
device *dev,
 	struct ata_host *host;
 	int i, j;
 
+	/* We must have at least one port info */
+	if (!ppi[0])
+		return NULL;
I've got to ask why on this one: most libata drivers use a static array
for the port info.  If the first element is NULL that's a coding
failure inside the driver, so WARN_ON would probably be more helpful to
the driver writer.

What makes the static checker think ppi isn't NULL?
quoted hunk ↗ jump to hunk
+
 	host = ata_host_alloc(dev, n_ports);
 	if (!host)
 		return NULL;
 
-	for (i = 0, j = 0, pi = NULL; i < host->n_ports; i++) {
+	for (i = 0, j = 0, pi = ppi[0]; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 
-		if (ppi[j])
-			pi = ppi[j++];
-
 		ap->pio_mask = pi->pio_mask;
 		ap->mwdma_mask = pi->mwdma_mask;
 		ap->udma_mask = pi->udma_mask;
@@ -5460,6 +5461,15 @@ struct ata_host *ata_host_alloc_pinfo(struct
device *dev,
 
 		if (!host->ops && (pi->port_ops !=
&ata_dummy_port_ops))
 			host->ops = pi->port_ops;
+
+		/*
+		 * Check that the next port info is not NULL.
+		 * If it is, keep using the current one.
+		 */
+		if (j < n_ports - 1 && ppi[j + 1]) {
+			j++;
+			pi = ppi[j];
+		}
This looks completely pointless: once you've verified ppi[0] is not
NULL above, there's no possible NULL deref in that loop and the static
checker should see it.  If it doesn't we need a new static checker
because we shouldn't be perturbing code for broken tools.

James

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help