[PATCH] drbd: Fix potential NULL pointer dereference in _drbd_set_state()

[PATCH] drbd: Fix potential NULL pointer dereference in _drbd_set_state()

[Ваторопин Андрей]

From: Andrey Vatoropin <a.vatoropin@crpt.ru>

The connection pointer receives a value in the _drbd_set_state()
function, including through a call to the first_peer_device() function.
This function returns a pointer to a list element. If the list is empty, it
returns a NULL pointer, which is later assigned to the connection
pointer. Subsequently, this pointer will be dereferenced.

Add a NULL check for the connection pointer to avoid dereferencing an
invalid pointer.

Found by Linux Verification Center (linuxtesting.org) with SVACE.
       
Fixes: a6b32bc3cebd ("drbd: Introduce "peer_device" object between "device" and "connection"")
Cc: stable@vger.kernel.org
Signed-off-by: Andrey Vatoropin <a.vatoropin@crpt.ru>
---
 drivers/block/drbd/drbd_state.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/block/drbd/drbd_state.c b/drivers/block/drbd/drbd_state.c
index adcba7f1d8ea..ea982d48017e 100644
--- a/drivers/block/drbd/drbd_state.c
+++ b/drivers/block/drbd/drbd_state.c
@@ -1281,6 +1281,11 @@ _drbd_set_state(struct drbd_device *device, union drbd_state ns,
 	if (rv < SS_SUCCESS)
 		return rv;
 
+	if (!connection) {
+		drbd_err(device, "No connection to peer, aborting!\n");
+		return SS_ALREADY_STANDALONE;
+	}
+
 	if (!(flags & CS_HARD)) {
 		/*  pre-state-change checks ; only look at ns  */
 		/* See drbd_state_sw_errors in drbd_strings.c */
-- 
2.43.0

Re: [PATCH] drbd: Fix potential NULL pointer dereference in _drbd_set_state()

[Christoph Böhmwalder]

Thanks for your patch.

On Thu, Jun 25, 2026 at 05:03:06AM +0000, Ваторопин Андрей wrote:
>From: Andrey Vatoropin <a.vatoropin@crpt.ru>
>
>The connection pointer receives a value in the _drbd_set_state()
>function, including through a call to the first_peer_device() function.
>This function returns a pointer to a list element. If the list is empty, it
>returns a NULL pointer, which is later assigned to the connection
>pointer. Subsequently, this pointer will be dereferenced.

Can the list actually be empty at this point?
The peer_device is linked into the list in drbd_create_device(), before
add_disk() and before the device is inserted into connection->peer_devices,
so no state change can reach the device earlier.

It is only unlinked again in drbd_destroy_device(), after the last kref
to the device is gone.
The connection itself is created together with the resource in
drbd_adm_new_resource() and lives until the resource is destroyed.

So for any device this function can be called on, first_peer_device()
returns a valid peer_device.

>
>Add a NULL check for the connection pointer to avoid dereferencing an
>invalid pointer.
>
>Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
>Fixes: a6b32bc3cebd ("drbd: Introduce "peer_device" object between "device" and "connection"")
>Cc: stable@vger.kernel.org
>Signed-off-by: Andrey Vatoropin <a.vatoropin@crpt.ru>
>---
> drivers/block/drbd/drbd_state.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
>diff --git a/drivers/block/drbd/drbd_state.c b/drivers/block/drbd/drbd_state.c
>index adcba7f1d8ea..ea982d48017e 100644
>--- a/drivers/block/drbd/drbd_state.c
>+++ b/drivers/block/drbd/drbd_state.c
>@@ -1281,6 +1281,11 @@ _drbd_set_state(struct drbd_device *device, union drbd_state ns,
> 	if (rv < SS_SUCCESS)
> 		return rv;
>
>+	if (!connection) {
>+		drbd_err(device, "No connection to peer, aborting!\n");
>+		return SS_ALREADY_STANDALONE;
>+	}
>+

Also, even if the condition could happen, its handling here would be 
wrong. Since this check happens before handling hard state changes, 
those could potentially be skipped, which is not allowed.
For example, after a local I/O error (drbd_chk_io_error), if this 
condition would trigger, the detach state change would be silently 
skipped. So in that circumstance, this patch would be actively harmful.

Also, SS_ALREADY_STANDALONE would map to the error message "Can not
disconnect a StandAlone device", which does not make any sense in this 
context.

> 	if (!(flags & CS_HARD)) {
> 		/*  pre-state-change checks ; only look at ns  */
> 		/* See drbd_state_sw_errors in drbd_strings.c */
>-- 
>2.43.0

In summary, unless I missed something major: NAK.

Thanks,
Christoph