IB/core: Fix potential NULL pointer dereference in pkey cache

PATCH Superseded Leon Romanovsky 3 participants 8 messages May 6, 2020 – May 7, 2020 InfiniBand

lore

Share & tools

lore

Leon Romanovsky

author

2020-05-06 05:32 UTC #0 lore raw

PATCH 1/1 IB/core: Fix potential NULL pointer dereference in pkey cache

From: Jack Morgenstein <[email protected]>

The IB core pkey cache is populated by procedure ib_cache_update(). Initially, the pkey cache pointer is NULL. ib_cache_update allocates a buffer and populates it with the device's pkeys, via repeated calls to procedure ib_query_pkey().

If there is a failure in populating the pkey buffer via ib_query_pkey(), ib_cache_update does not replace the old pkey buffer cache with the updated one -- it leaves the old cache as is.

Since initially the pkey buffer cache is NULL, when calling ib_cache_update the first time, a failure in ib_query_pkey() will cause the pkey buffer cache pointer to remain NULL.

In this situation, any calls subsequent to ib_get_cached_pkey(), ib_find_cached_pkey(), or ib_find_cached_pkey_exact() will try to dereference the NULL pkey cache pointer, causing a kernel panic.

Fix this by checking the ib_cache_update() return value.

Fixes 8faea9fd4a39 ("RDMA/cache: Move the cache per-port data into the main ib_port_data")

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")

Signed-off-by Jack Morgenstein <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
Changelog:
v1: I rewrote the patch to take care of ib_cache_update() return value.
v0: https://lore.kernel.org/linux-rdma/[email protected]

---
 drivers/infiniband/core/cache.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

drivers/infiniband/core/cache.c +9 -2

index 717b798cddad..1cbebfa374a5 100644
@@ -1553,10 +1553,17 @@ int ib_cache_setup_one(struct ib_device *device)
 	if (err)
 		return err;
 
-	rdma_for_each_port (device, p)
-		ib_cache_update(device, p, true);
+	rdma_for_each_port (device, p) {
+		err = ib_cache_update(device, p, true);
+		if (err)
+			goto out;
+	}
 
 	return 0;
+
+out:
+	ib_cache_release_one(device);
+	return err;
 }
 
 void ib_cache_release_one(struct ib_device *device)

--
2.26.2

Jason Gunthorpe

2020-05-06 14:43 UTC #1 lore raw

Quoted 47 lines · Leon Romanovsky

From: Jack Morgenstein <[email protected]>
The IB core pkey cache is populated by procedure ib_cache_update(). Initially, the pkey cache pointer is NULL. ib_cache_update allocates a buffer and populates it with the device's pkeys, via repeated calls to procedure ib_query_pkey().
If there is a failure in populating the pkey buffer via ib_query_pkey(), ib_cache_update does not replace the old pkey buffer cache with the updated one -- it leaves the old cache as is.
Since initially the pkey buffer cache is NULL, when calling ib_cache_update the first time, a failure in ib_query_pkey() will cause the pkey buffer cache pointer to remain NULL.
In this situation, any calls subsequent to ib_get_cached_pkey(), ib_find_cached_pkey(), or ib_find_cached_pkey_exact() will try to dereference the NULL pkey cache pointer, causing a kernel panic.
Fix this by checking the ib_cache_update() return value.
Fixes 8faea9fd4a39 ("RDMA/cache: Move the cache per-port data into the main ib_port_data")
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by Jack Morgenstein <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
Changelog:
v1: I rewrote the patch to take care of ib_cache_update() return value.
v0: https://lore.kernel.org/linux-rdma/[email protected]
---
 drivers/infiniband/core/cache.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
--
2.26.2
drivers/infiniband/core/cache.c +1 -2
index 717b798cddad..1cbebfa374a5 100644
@@ -1553,10 +1553,17 @@ int ib_cache_setup_one(struct ib_device *device)
 	if (err)
 		return err;
 
-	rdma_for_each_port (device, p)
-		ib_cache_update(device, p, true);
+	rdma_for_each_port (device, p) {

drivers/infiniband/core/cache.c int ib_cache_setup_one(struct ib_device *device) · line 1566

Quoted 10 lines

drivers/infiniband/core/cache.c +8 -0

+		err = ib_cache_update(device, p, true);
+		if (err)
+			goto out;
+	}
 
 	return 0;
+
+out:
+	ib_cache_release_one(device);
+	return err;

ib_cache_release_once can be called only once, and it is always called by ib_device_release(), it should not be called here

Jason

Leon Romanovsky

author

2020-05-06 16:56 UTC #2 lore raw

Quoted 53 lines · Jason Gunthorpe

Quoted 52 lines · Leon Romanovsky
From: Jack Morgenstein <[email protected]>
The IB core pkey cache is populated by procedure ib_cache_update(). Initially, the pkey cache pointer is NULL. ib_cache_update allocates a buffer and populates it with the device's pkeys, via repeated calls to procedure ib_query_pkey().
If there is a failure in populating the pkey buffer via ib_query_pkey(), ib_cache_update does not replace the old pkey buffer cache with the updated one -- it leaves the old cache as is.
Since initially the pkey buffer cache is NULL, when calling ib_cache_update the first time, a failure in ib_query_pkey() will cause the pkey buffer cache pointer to remain NULL.
In this situation, any calls subsequent to ib_get_cached_pkey(), ib_find_cached_pkey(), or ib_find_cached_pkey_exact() will try to dereference the NULL pkey cache pointer, causing a kernel panic.
Fix this by checking the ib_cache_update() return value.
Fixes 8faea9fd4a39 ("RDMA/cache: Move the cache per-port data into the main ib_port_data")
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by Jack Morgenstein <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
Changelog:
v1: I rewrote the patch to take care of ib_cache_update() return value.
v0: https://lore.kernel.org/linux-rdma/[email protected]
---
 drivers/infiniband/core/cache.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
--
2.26.2
drivers/infiniband/core/cache.c +5 -2
index 717b798cddad..1cbebfa374a5 100644
@@ -1553,10 +1553,17 @@ int ib_cache_setup_one(struct ib_device *device)
 	if (err)
 		return err;
 
-	rdma_for_each_port (device, p)
-		ib_cache_update(device, p, true);
+	rdma_for_each_port (device, p) {
+		err = ib_cache_update(device, p, true);
+		if (err)
+			goto out;
+	}
 

Quoted 8 lines

drivers/infiniband/core/cache.c int ib_cache_setup_one(struct ib_device *device) · line 1566
Quoted 5 lines
drivers/infiniband/core/cache.c +4 -0
 	return 0;
+
+out:
+	ib_cache_release_one(device);
+	return err;
ib_cache_release_once can be called only once, and it is always called by ib_device_release(), it should not be called here

It doesn't sound right if we rely on ib_device_release() to unwind error in ib_cache_setup_one(). I don't think that we need to return from ib_cache_setup_one() without cleaning it.

Thanks

Quoted 2 lines

Jason

Jason Gunthorpe

2020-05-06 18:09 UTC #3 lore raw

Quoted 58 lines · Leon Romanovsky

Quoted 48 lines · Jason Gunthorpe
Quoted 47 lines · Leon Romanovsky
From: Jack Morgenstein <[email protected]>
The IB core pkey cache is populated by procedure ib_cache_update(). Initially, the pkey cache pointer is NULL. ib_cache_update allocates a buffer and populates it with the device's pkeys, via repeated calls to procedure ib_query_pkey().
If there is a failure in populating the pkey buffer via ib_query_pkey(), ib_cache_update does not replace the old pkey buffer cache with the updated one -- it leaves the old cache as is.
Since initially the pkey buffer cache is NULL, when calling ib_cache_update the first time, a failure in ib_query_pkey() will cause the pkey buffer cache pointer to remain NULL.
In this situation, any calls subsequent to ib_get_cached_pkey(), ib_find_cached_pkey(), or ib_find_cached_pkey_exact() will try to dereference the NULL pkey cache pointer, causing a kernel panic.
Fix this by checking the ib_cache_update() return value.
Fixes 8faea9fd4a39 ("RDMA/cache: Move the cache per-port data into the main ib_port_data")
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by Jack Morgenstein <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
Changelog:
v1: I rewrote the patch to take care of ib_cache_update() return value.
v0: https://lore.kernel.org/linux-rdma/[email protected]
 drivers/infiniband/core/cache.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/infiniband/core/cache.c +6 -2
index 717b798cddad..1cbebfa374a5 100644
+++ b/drivers/infiniband/core/cache.c
@@ -1553,10 +1553,17 @@ int ib_cache_setup_one(struct ib_device *device)
 	if (err)
 		return err;
 
-	rdma_for_each_port (device, p)
-		ib_cache_update(device, p, true);
+	rdma_for_each_port (device, p) {
+		err = ib_cache_update(device, p, true);
+		if (err)
+			goto out;
+	}
 
Quoted 8 lines
drivers/infiniband/core/cache.c int ib_cache_setup_one(struct ib_device *device) · line 1566
Quoted 5 lines
drivers/infiniband/core/cache.c +4 -0
 	return 0;
+
+out:
+	ib_cache_release_one(device);
+	return err;
ib_cache_release_once can be called only once, and it is always called by ib_device_release(), it should not be called here

Quoted 3 lines

It doesn't sound right if we rely on ib_device_release() to unwind error in ib_cache_setup_one(). I don't think that we need to return from ib_cache_setup_one() without cleaning it.

We do as ib_cache_release_one() cannot be called multiple times

The general design of all this pre-registration stuff is that the release function does the clean up and the individual functions should not error unwind cleanup done in the unconditional release.

Other schemes were too complicated

Jason

Leon Romanovsky

author

2020-05-06 18:31 UTC #4 lore raw

Quoted 59 lines · Jason Gunthorpe

Quoted 58 lines · Leon Romanovsky
Quoted 56 lines · Jason Gunthorpe
Quoted 42 lines · Leon Romanovsky
From: Jack Morgenstein <[email protected]>
The IB core pkey cache is populated by procedure ib_cache_update(). Initially, the pkey cache pointer is NULL. ib_cache_update allocates a buffer and populates it with the device's pkeys, via repeated calls to procedure ib_query_pkey().
If there is a failure in populating the pkey buffer via ib_query_pkey(), ib_cache_update does not replace the old pkey buffer cache with the updated one -- it leaves the old cache as is.
Since initially the pkey buffer cache is NULL, when calling ib_cache_update the first time, a failure in ib_query_pkey() will cause the pkey buffer cache pointer to remain NULL.
In this situation, any calls subsequent to ib_get_cached_pkey(), ib_find_cached_pkey(), or ib_find_cached_pkey_exact() will try to dereference the NULL pkey cache pointer, causing a kernel panic.
Fix this by checking the ib_cache_update() return value.
Fixes 8faea9fd4a39 ("RDMA/cache: Move the cache per-port data into the main ib_port_data")
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by Jack Morgenstein <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
Changelog:
v1: I rewrote the patch to take care of ib_cache_update() return value.
v0: https://lore.kernel.org/linux-rdma/[email protected]
 drivers/infiniband/core/cache.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/infiniband/core/cache.c +2 -2
index 717b798cddad..1cbebfa374a5 100644
+++ b/drivers/infiniband/core/cache.c
@@ -1553,10 +1553,17 @@ int ib_cache_setup_one(struct ib_device *device)
 	if (err)
 		return err;
 
-	rdma_for_each_port (device, p)
-		ib_cache_update(device, p, true);
+	rdma_for_each_port (device, p) {
drivers/infiniband/core/cache.c int ib_cache_setup_one(struct ib_device *device) · line 1566
Quoted 10 lines
drivers/infiniband/core/cache.c +8 -0
+		err = ib_cache_update(device, p, true);
+		if (err)
+			goto out;
+	}
 
 	return 0;
+
+out:
+	ib_cache_release_one(device);
+	return err;
ib_cache_release_once can be called only once, and it is always called by ib_device_release(), it should not be called here

Quoted 5 lines

Quoted 3 lines
It doesn't sound right if we rely on ib_device_release() to unwind error in ib_cache_setup_one(). I don't think that we need to return from ib_cache_setup_one() without cleaning it.
We do as ib_cache_release_one() cannot be called multiple times

Do you want me to respin?

Quoted 6 lines

The general design of all this pre-registration stuff is that the release function does the clean up and the individual functions should not error unwind cleanup done in the unconditional release.
Other schemes were too complicated

It doesn't mean that it is right :)

Thanks

Quoted 2 lines

Jason

jackm

2020-05-06 18:41 UTC #5 lore raw

In reply to Jason Gunthorpe

Quoted 13 lines · On Wed, 6 May 2020 15:09:36 -0300 Jason Gunthorpe <[email protected]> wrote:

Quoted 10 lines
Quoted 6 lines
drivers/infiniband/core/cache.c int ib_cache_setup_one(struct ib_device *device) · line 1566
Quoted 3 lines
drivers/infiniband/core/cache.c +3 -0
+out:
+	ib_cache_release_one(device);
+	return err;  
ib_cache_release_once can be called only once, and it is always called by ib_device_release(), it should not be called here
It doesn't sound right if we rely on ib_device_release() to unwind error in ib_cache_setup_one(). I don't think that we need to return from ib_cache_setup_one() without cleaning it.
We do as ib_cache_release_one() cannot be called multiple times

Quoted 7 lines

The general design of all this pre-registration stuff is that the release function does the clean up and the individual functions should not error unwind cleanup done in the unconditional release.
Other schemes were too complicated
Jason

What about calling gid_table_release_one(device) instead of ib_cache_release_one(device) in the error flow ?

gid_table_release_one() calls gid_table_release -- which frees the gid table and sets its pointer to NULL.

Then, if ib_cache_release_one is called later, gid_table_release_one() will simply do nothing (it calls gid_table_release, which returns without doing anything if the table pointer argument is NULL -- which it will be).

Thus, unlike ib_cache_release_one() -- gid_table_release_one() is callable multiple times.

This also has the advantage of unwinding the gid_table_setup_one() in the ib_cache_setup_one() error flow -- which is a symmetric unwind.

-Jack

Jason Gunthorpe

2020-05-06 18:57 UTC #6 lore raw

Quoted 20 lines · jackm

Quoted 17 lines · On Wed, 6 May 2020 15:09:36 -0300 Jason Gunthorpe <[email protected]> wrote:
Quoted 10 lines
Quoted 6 lines
drivers/infiniband/core/cache.c int ib_cache_setup_one(struct ib_device *device) · line 1566
Quoted 3 lines
drivers/infiniband/core/cache.c +3 -0
+out:
+	ib_cache_release_one(device);
+	return err;  
ib_cache_release_once can be called only once, and it is always called by ib_device_release(), it should not be called here
It doesn't sound right if we rely on ib_device_release() to unwind error in ib_cache_setup_one(). I don't think that we need to return from ib_cache_setup_one() without cleaning it.
We do as ib_cache_release_one() cannot be called multiple times
The general design of all this pre-registration stuff is that the release function does the clean up and the individual functions should not error unwind cleanup done in the unconditional release.

Quoted 6 lines

Quoted 3 lines
Other schemes were too complicated
Jason
What about calling gid_table_release_one(device) instead of ib_cache_release_one(device) in the error flow ?

Why?

That is not the design, everything that is freed by release is defered to release, even on error paths.

Jason

Leon Romanovsky

author

2020-05-07 05:58 UTC #7 lore raw

Quoted 21 lines · Jason Gunthorpe

Quoted 20 lines · jackm
Quoted 17 lines · On Wed, 6 May 2020 15:09:36 -0300 Jason Gunthorpe <[email protected]> wrote:
Quoted 10 lines
Quoted 6 lines
drivers/infiniband/core/cache.c int ib_cache_setup_one(struct ib_device *device) · line 1566
Quoted 3 lines
drivers/infiniband/core/cache.c +3 -0
+out:
+	ib_cache_release_one(device);
+	return err;
ib_cache_release_once can be called only once, and it is always called by ib_device_release(), it should not be called here
It doesn't sound right if we rely on ib_device_release() to unwind error in ib_cache_setup_one(). I don't think that we need to return from ib_cache_setup_one() without cleaning it.
We do as ib_cache_release_one() cannot be called multiple times
The general design of all this pre-registration stuff is that the release function does the clean up and the individual functions should not error unwind cleanup done in the unconditional release.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Other schemes were too complicated
Jason
What about calling gid_table_release_one(device) instead of ib_cache_release_one(device) in the error flow ?
Why?

Because it doesn't look clean.

Quoted 3 lines

That is not the design, everything that is freed by release is defered to release, even on error paths.

I'll resend now.

Thanks

Quoted 2 lines

Jason