[PATCH 0/2] drbd: validate payload sizes received from the peer

[PATCH 0/2] drbd: validate payload sizes received from the peer

[Yibo Liu]

Two places in the DRBD receive path take a payload size from the peer's
packet header and use it without comparing it to the size they expect.
Commit bd910a7660d2 ("drbd: reject data replies with an out-of-range
payload size") already rejects negative sizes in recv_dless_read();
these patches cover the remaining cases:

1/2 rejects a P_DATA_REPLY whose size differs from the read request.
    A short reply currently completes the read successfully with part
    of the bio never written.

2/2 rejects P_CSUM_RS_REQUEST and P_OV_REPLY packets whose digest
    length does not match the configured algorithm, and makes the
    worker compare digests only when their sizes match. A short digest
    currently causes an out-of-bounds read in memcmp(), and a huge one
    triggers a page allocator warning.

Like the earlier fixes, this only matters for a peer that does not
follow the protocol; a correct peer always sends the expected sizes.

The problems were found with the help of an AI coding assistant and
were reproduced on block-7.3 before posting (details below the "---" of
each patch). Testing:
- before/after runs of the reproducer in an arm64 virtme-ng KASAN guest
- a complete csums-alg resync and an online verify as regression tests
- each patch built with W=1 and sparse (C=2), arm64, gcc 13.3: no new
  warnings
- checkpatch.pl --strict clean
Not built with clang or for other architectures.

Yibo Liu (2):
  drbd: reject data replies whose size does not match the request
  drbd: validate digest size in checksum resync and verify requests

 drivers/block/drbd/drbd_receiver.c | 14 ++++++++++++--
 drivers/block/drbd/drbd_worker.c   |  8 ++++----
 2 files changed, 16 insertions(+), 6 deletions(-)


base-commit: 41cab3a71dc7be62114d91cc849424879062d324
-- 
2.39.5 (Apple Git-154)

[PATCH 1/2] drbd: reject data replies whose size does not match the request

[Yibo Liu]

A node that reads from its peer (it has no local disk, the area is not
up to date locally, or read-balancing chose the peer) sends a
P_DATA_REQUEST for req->i.size bytes, and recv_dless_read() copies the
matching P_DATA_REPLY into the original bio. Commit bd910a7660d2
("drbd: reject data replies with an out-of-range payload size") rejects
a negative payload size, but a reply that is shorter or longer than the
request is still accepted.

A shorter reply only fills the start of the bio. data_size still drops
to zero in the copy loop, so the final D_ASSERT() does not fire, and
the read completes successfully although the rest of the caller's
buffer was never written. With a proxy that cut one reply down to
1 KiB, a 4 KiB O_DIRECT read returned 4096 with the last 3 KiB of the
buffer unchanged and nothing in the kernel log. A longer reply leaves
the excess in the socket, where it is parsed as the next packet header.

A correct peer always replies with exactly the requested size. Reject
any other size, as is already done for negative sizes.

Fixes: b411b3637fa7 ("The DRBD driver")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Yibo Liu <yiboliume@gmail.com>
---

Notes:
    - Reproduced on block-7.3 (41cab3a71dc7) in an arm64 virtme-ng guest,
      with two DRBD 8.4 resources connected over loopback through a proxy
      that rewrites packets. A diskless Primary read 4 KiB with O_DIRECT
      while the proxy cut the P_DATA_REPLY to 1 KiB of data. Before: read()
      returned 4096, the last 3072 bytes of the buffer were never written,
      and nothing was logged. After: "Invalid data reply size 1024, expected
      4096", the connection is dropped and the read fails with -EIO.
      Unmodified 4 KiB reads succeed before and after. The harness is
      available on request.
    - recv_dless_read() in LINBIT's DRBD 9 tree, which the pending DRBD 9
      rework series is based on, has the same check, so the series probably
      needs this change too. I have not checked the posted series itself.

 drivers/block/drbd/drbd_receiver.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c
index 2135c14354a8..97b306676626 100644
--- a/drivers/block/drbd/drbd_receiver.c
+++ b/drivers/block/drbd/drbd_receiver.c
@@ -1810,8 +1810,9 @@ static int recv_dless_read(struct drbd_peer_device *peer_device, struct drbd_req
 		data_size -= digest_size;
 	}
 
-	if (data_size < 0) {
-		drbd_err(peer_device, "Invalid data reply size\n");
+	if (data_size < 0 || data_size != req->i.size) {
+		drbd_err(peer_device, "Invalid data reply size %d, expected %u\n",
+			 data_size, req->i.size);
 		return -EIO;
 	}
 
-- 
2.39.5 (Apple Git-154)

[PATCH 2/2] drbd: validate digest size in checksum resync and verify requests

[Yibo Liu]

receive_DataRequest() takes the digest length of P_CSUM_RS_REQUEST and
P_OV_REPLY packets from the packet header and passes it to kmalloc()
without any check. The worker later compares the received digest with
one computed by the local csums or verify algorithm. A size mismatch is
only reported by D_ASSERT(), and memcmp() runs anyway.

A digest that is shorter than the algorithm's digest size therefore
makes w_e_end_csum_rs_req() and w_e_end_ov_reply() read past the end of
the digest_info allocation:

  BUG: KASAN: slab-out-of-bounds in memcmp+0x1b8/0x1d8
  Read of size 8 at addr ffff00000a638610 by task drbd_w_r1/212
  Call trace:
   memcmp
   w_e_end_csum_rs_req
   drbd_worker
  Allocated by task 240:
   __kmalloc_noprof
   receive_DataRequest
   drbd_receiver

A digest length larger than the page allocator can serve triggers the
WARN_ON_ONCE() for oversized allocations, as GFP_NOIO does not include
__GFP_NOWARN:

  WARNING: mm/page_alloc.c:5340 at __alloc_frozen_pages_noprof
  Call trace:
   ___kmalloc_large_node
   __kmalloc_noprof
   receive_DataRequest
   drbd_receiver

Reject these packets unless the digest length matches the digest size
of the configured csums or verify algorithm, and also reject them when
no such algorithm is configured. In the worker, only compare digests of
the same size: the peer can switch csums-alg or verify-alg with
P_SYNC_PARAM after the request was received, and a size mismatch is
then treated as "not in sync" instead of reading out of bounds.

Fixes: b411b3637fa7 ("The DRBD driver")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Yibo Liu <yiboliume@gmail.com>
---

Notes:
    - Reproduced in the same setup with csums-alg=sha1, the proxy rewriting
      one P_CSUM_RS_REQUEST: a 0-byte digest gives the KASAN report above,
      a 1 GiB digest length gives the WARNING above. With the patch both are
      rejected with "CsumRSRequest: unexpected digest size ..." and there is
      no KASAN report or warning. A complete csums-alg resync followed by an
      online verify with verify-alg=sha1, without tampering, still passes.
      The harness is available on request.
    - In LINBIT's DRBD 9 tree, receive_digest() only rejects negative sizes
      and the sender keeps the D_ASSERT() before memcmp(), so the pending
      DRBD 9 rework series probably needs the same checks. I have not
      checked the posted series itself.

 drivers/block/drbd/drbd_receiver.c | 9 +++++++++
 drivers/block/drbd/drbd_worker.c   | 8 ++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c
index 97b306676626..87e714477050 100644
--- a/drivers/block/drbd/drbd_receiver.c
+++ b/drivers/block/drbd/drbd_receiver.c
@@ -2629,6 +2629,7 @@ static int receive_DataRequest(struct drbd_connection *connection, struct packet
 	sector_t capacity;
 	struct drbd_peer_request *peer_req;
 	struct digest_info *di = NULL;
+	struct crypto_shash *tfm;
 	int size, verb;
 	struct p_block_req *p =	pi->data;
 
@@ -2713,6 +2714,14 @@ static int receive_DataRequest(struct drbd_connection *connection, struct packet
 
 	case P_OV_REPLY:
 	case P_CSUM_RS_REQUEST:
+		tfm = pi->cmd == P_OV_REPLY ? connection->verify_tfm :
+					      connection->csums_tfm;
+		if (!tfm || pi->size != crypto_shash_digestsize(tfm)) {
+			drbd_err(device, "%s: unexpected digest size %u\n",
+				 cmdname(pi->cmd), pi->size);
+			goto out_free_e;
+		}
+
 		di = kmalloc(sizeof(*di) + pi->size, GFP_NOIO);
 		if (!di)
 			goto out_free_e;
diff --git a/drivers/block/drbd/drbd_worker.c b/drivers/block/drbd/drbd_worker.c
index 0697f99fed18..31e54960c5de 100644
--- a/drivers/block/drbd/drbd_worker.c
+++ b/drivers/block/drbd/drbd_worker.c
@@ -1174,8 +1174,8 @@ int w_e_end_csum_rs_req(struct drbd_work *w, int cancel)
 		 * introducing more locking mechanisms */
 		if (peer_device->connection->csums_tfm) {
 			digest_size = crypto_shash_digestsize(peer_device->connection->csums_tfm);
-			D_ASSERT(device, digest_size == di->digest_size);
-			digest = kmalloc(digest_size, GFP_NOIO);
+			if (digest_size == di->digest_size)
+				digest = kmalloc(digest_size, GFP_NOIO);
 		}
 		if (digest) {
 			drbd_csum_ee(peer_device->connection->csums_tfm, peer_req, digest);
@@ -1301,8 +1301,8 @@ int w_e_end_ov_reply(struct drbd_work *w, int cancel)
 		if (digest) {
 			drbd_csum_ee(peer_device->connection->verify_tfm, peer_req, digest);
 
-			D_ASSERT(device, digest_size == di->digest_size);
-			eq = !memcmp(digest, di->digest, digest_size);
+			eq = digest_size == di->digest_size &&
+			     !memcmp(digest, di->digest, digest_size);
 			kfree(digest);
 		}
 	}
-- 
2.39.5 (Apple Git-154)