[PATCH 0/2] nvme-tcp: two checks on the C2HData receive path

[Yehyeong Lee]

Two independent problems on the nvme-tcp host receive path, both reachable
by a controller sending a C2HData PDU that does not match the command it
refers to.

1/2: the host never compares the total number of bytes received against the
length the command asked for, so a controller can answer a 4096-byte read
with 512 bytes and have the request complete as fully successful.

2/2: nvme_tcp_handle_c2h_data() still gates on blk_rq_payload_bytes()
without first checking blk_rq_nr_phys_segments(), which is the rule
established for the send path by commit 25e5cb780e62 ("nvme-tcp: fix
possible crash in write_zeroes processing").

Both were reproduced against an unpatched v7.2-rc5 host using a test target
that sends the malformed PDUs as ordinary wire traffic. The same target and
guest program show the checks holding afterwards, and target arms that
behave correctly are unaffected, including a controller that splits its
response across two C2HData PDUs.

All measurements are single-environment: one QEMU guest against a test
target on loopback, with no real controller and no independent reviewer.

Patch 1/2 adds a dereference of the request at the digest completion
site, where nvme_tcp_recv_ddgst() resolves the command id with
nvme_cid_to_rq() rather than nvme_find_rq(). That site and
nvme_tcp_recv_data() are only reached after nvme_tcp_handle_c2h_data()
has already resolved the same command id with nvme_find_rq(), which
does check for NULL, so no unvalidated request is dereferenced.

Two questions for the maintainers:

- 1/2 returns -EPROTO, which nvme_tcp_recv_skb() turns into error recovery,
  i.e. a controller reset rather than a single failed command. The rejected
  request does not hang: the block layer retries, pread() then returns EIO,
  and the command completes with NVME_SC_HOST_ABORTED_CMD, with no hung
  task reported. It is slow, 688 s in my measurement, because the retries
  are spread across reconnect attempts - which is what this driver already
  does for any receive-path error, including the "unexpected data" -EIO
  sitting next to it in nvme_tcp_handle_c2h_data(). Failing only the
  command would be gentler but would diverge from the other
  protocol-violation returns in this file. Preference?

- 1/2's direction gate is rq_data_dir(rq) == READ, which REQ_OP_DRV_IN
  folds into, so passthrough reads are covered too. That is deliberate,
  but if a device returns fewer bytes than a user-supplied buffer for a
  vendor command the result becomes a controller reset. Adding
  blk_rq_is_passthrough() as a fourth gate would avoid that at the cost of
  leaving passthrough reads unprotected. I have not tested a passthrough
  read against a short-answering controller.

Yehyeong Lee (2):
  nvme-tcp: reject a data-in command that transferred too few bytes
  nvme-tcp: do not accept C2HData based on blk_rq_payload_bytes() alone

 drivers/nvme/host/tcp.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

-- 
2.43.0