iscsi-scstd: Validate complete iSNS payloads before consuming them

Manual review found missing bounds checks in print_scn_pdu and
qry_rsp_handle; no saved analyzer diagnostic directly reports these
bugs. Both handlers read a TLV header before proving it is present.
Clamping an oversized value can underflow the remaining length or
accept a truncated attribute. Query handling also reads a missing
status word and can discard the old ACL before noticing bad input.

Validate every TLV header, value length and alignment before processing
any attributes. Check and decode the response status in host byte order.
Reject malformed tails before modifying names or replacing the ACL.

Add an isolated Ginkgo probe of both production handlers with payloads
ending at a guard page. Cover short headers/status, oversized and
unaligned values, malformed tails, delimiters and valid ACL replacement.

iscsi-scst/usr/isns.c

@@ -772,6 +772,23 @@ static int recv_pdu(int fd, struct isns_io *rx, struct isns_hdr *hdr)
 		  function, length, flags, transaction, sequence);	\
 }
 
+/* Validate the complete payload before a handler changes names or ACLs. */
+static bool isns_tlvs_valid(const struct isns_tlv *tlv, size_t length)
+{
+	while (length) {
+		uint32_t vlen;
+
+		if (length < sizeof(*tlv))
+			return false;
+		vlen = ntohl(tlv->length);
+		if (vlen > length - sizeof(*tlv) || vlen % ISNS_ALIGN != 0)
+			return false;
+		length -= sizeof(*tlv) + vlen;
+		tlv = (const struct isns_tlv *)((const char *)tlv->value + vlen);
+	}
+	return true;
+}
+
 static char *print_scn_pdu(struct isns_hdr *hdr)
 {
 	struct isns_tlv *tlv = (struct isns_tlv *)hdr->pdu;
@@ -783,13 +800,14 @@ static char *print_scn_pdu(struct isns_hdr *hdr)
 	char *name = NULL;
 
 	get_hdr_param(hdr, function, length, flags, transaction, sequence);
+	if (!isns_tlvs_valid(tlv, length)) {
+		log_error("Malformed iSNS SCN attributes");
+		return NULL;
+	}
 
 	while (length) {
 		uint32_t vlen = ntohl(tlv->length);
 
-		if (vlen + sizeof(*tlv) > length)
-			vlen = length - sizeof(*tlv);
-
 		if (vlen < 4)
 			goto next;
 
@@ -826,7 +844,7 @@ static void qry_rsp_handle(struct isns_hdr *hdr)
 	uint16_t flags __attribute__((unused));
 	uint16_t sequence __attribute__((unused));
 	uint16_t length, transaction;
-	uint32_t status = (uint32_t) (*hdr->pdu);
+	uint32_t status;
 	struct isns_qry_mgmt *mgmt, *n;
 	struct target *target;
 	struct isns_initiator *ini;
@@ -847,6 +865,11 @@ static void qry_rsp_handle(struct isns_hdr *hdr)
 	return;
 
 found:
+	if (length < sizeof(status)) {
+		log_error("Short iSNS query response status");
+		goto free_qry_mgmt;
+	}
+	status = ntohl(*hdr->pdu);
 	if (status) {
 		log_error("%s %d: error response %u",
 			  __func__, __LINE__, status);
@@ -854,6 +877,14 @@ found:
 		goto free_qry_mgmt;
 	}
 
+	/* Skip the status only after proving that it is present. */
+	tlv = (struct isns_tlv *)((char *)hdr->pdu + sizeof(status));
+	length -= sizeof(status);
+	if (!isns_tlvs_valid(tlv, length)) {
+		log_error("Malformed iSNS query response attributes");
+		goto free_qry_mgmt;
+	}
+
 	if (!strlen(mgmt->name)) {
 		log_debug(1, "%s %d: skip %u",
 			  __func__, __LINE__, transaction);
@@ -869,18 +900,9 @@ found:
 
 	free_all_acl(target);
 
-	/* skip status */
-	if (length < 4)
-		goto free_qry_mgmt;
-	tlv = (struct isns_tlv *)((char *)hdr->pdu + 4);
-	length -= 4;
-
 	while (length) {
 		uint32_t vlen = ntohl(tlv->length);
 
-		if (vlen + sizeof(*tlv) > length)
-			vlen = length - sizeof(*tlv);
-
 		if (vlen < 4)
 			goto next;
 

tests/ginkgo/README.md

@@ -127,6 +127,11 @@ failure cleanup. It runs through `ginkgo-check` and requires `cc`. Interface
 headers are prepared in a temporary directory; no daemon or storage is used.
 Run it separately with `go test ./internal/chap` from this directory.
 
+The isolated `internal/isns` suite checks production iSNS handlers against
+malformed payloads ending at a guard page, including preservation of existing
+ACLs. It also runs through `ginkgo-check`; use `go test ./internal/isns` to
+run it separately. It requires `cc` and opens no sockets.
+
 ## SCSI and I/O
 
 Install Python 3, util-linux and sg3_utils on the command host: the target for

tests/ginkgo/internal/isns/isns_test.go

@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0-only
+package isns
+
+import (
+	_ "embed"
+	"os"
+	"path/filepath"
+	"runtime"
+	"testing"
+	"time"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	"scst-component-tests/internal/admin"
+	"scst-component-tests/internal/cprobe"
+)
+
+//go:embed testdata/tlv_probe.c
+var tlvProbe string
+
+func TestISNS(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "SCST iSNS payload bounds")
+}
+
+var _ = Describe("iSNS payload validation", Label("unit", "userspace", "isns"), func() {
+	var probe string
+	var client *admin.Client
+
+	BeforeEach(func(ctx SpecContext) {
+		client = &admin.Client{Log: GinkgoWriter}
+		dir, err := os.MkdirTemp("", "scst-isns-probe-")
+		Expect(err).NotTo(HaveOccurred())
+		DeferCleanup(func() {
+			if client.Blocked() != nil {
+				AddReportEntry("preserved iSNS probe", dir)
+				return
+			}
+			Expect(os.RemoveAll(dir)).To(Succeed())
+		})
+		source := os.Getenv("SCST_TEST_SOURCE_DIR")
+		if source == "" {
+			_, file, _, ok := runtime.Caller(0)
+			Expect(ok).To(BeTrue())
+			source = filepath.Clean(filepath.Join(filepath.Dir(file), "../../../.."))
+		}
+		Expect(cprobe.PrepareISCSIHeaders(source, dir)).To(Succeed())
+		input := filepath.Join(dir, "tlv_probe.c")
+		Expect(os.WriteFile(input, []byte(tlvProbe), 0600)).To(Succeed())
+		probe = filepath.Join(dir, "tlv-probe")
+		// Dead-section removal keeps this unit probe independent of the
+		// daemon's unrelated authentication and transport dependencies.
+		r, err := client.RunProgram(ctx, 30*time.Second,
+			"cc", "-D_GNU_SOURCE", "-O2", "-ffunction-sections", "-fdata-sections",
+			"-I"+dir,
+			"-I"+filepath.Join(source, "iscsi-scst/usr"),
+			"-I"+filepath.Join(source, "iscsi-scst/include"),
+			"-I"+filepath.Join(source, "scst/include"),
+			input, "-Wl,--gc-sections", "-o", probe)
+		Expect(err).NotTo(HaveOccurred(), r.String())
+		Expect(r.ExitCode).To(BeZero(), r.String())
+	})
+
+	DescribeTable("checks bounds before reading or replacing ACLs", func(ctx SpecContext, scenario string) {
+		r, err := client.RunProgram(ctx, 10*time.Second, probe, scenario)
+		Expect(err).NotTo(HaveOccurred(), r.String())
+		Expect(r.ExitCode).To(BeZero(), r.String())
+		Expect(r.Stdout).To(Equal("ok\n"), r.String())
+	},
+		Entry("empty and short SCN payloads stop at the guard page", "scn-short"),
+		Entry("oversized SCN value length is rejected", "scn-oversized"),
+		Entry("truncated SCN value is rejected", "scn-truncated"),
+		Entry("unaligned SCN value is rejected", "scn-unaligned"),
+		Entry("malformed SCN tail does not modify the earlier name", "scn-tail"),
+		Entry("valid SCN name and bitmap are accepted", "scn-valid"),
+		Entry("zero-length delimiter advances to the next attribute", "scn-delimiter"),
+		Entry("short query status preserves the ACL", "query-short"),
+		Entry("short query TLV header preserves the ACL", "query-short-tlv"),
+		Entry("oversized query value preserves the ACL", "query-oversized"),
+		Entry("malformed query tail preserves the complete ACL", "query-tail"),
+		Entry("error status preserves the ACL", "query-error"),
+		Entry("valid query replaces the ACL", "query-valid"),
+	)
+})

tests/ginkgo/internal/isns/testdata/tlv_probe.c

@@ -0,0 +1,194 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
+/* Keep both production handlers; no socket or daemon entry point is invoked. */
+#include "isns.c"
+
+static void check(bool condition, int line, const char *expression)
+{
+	if (!condition) {
+		fprintf(stderr, "line %d: %s\n", line, expression);
+		exit(EXIT_FAILURE);
+	}
+}
+
+#define CHECK(condition) check(!!(condition), __LINE__, #condition)
+
+static struct target probe_target;
+
+void __log(const char *func, int line, int prio, int level, const char *fmt, ...)
+{
+}
+
+struct target *target_find_by_name(const char *name)
+{
+	return strcmp(name, probe_target.name) == 0 ? &probe_target : NULL;
+}
+
+struct guarded_pdu {
+	void *mapping;
+	size_t size;
+	struct isns_hdr *hdr;
+};
+
+static int make_pdu(struct guarded_pdu *pdu, size_t length)
+{
+	long page = sysconf(_SC_PAGESIZE);
+
+	CHECK(page > 0 && length + sizeof(*pdu->hdr) < (size_t)page);
+	pdu->size = 2 * (size_t)page;
+	pdu->mapping = mmap(NULL, pdu->size, PROT_READ | PROT_WRITE,
+			    MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	CHECK(pdu->mapping != MAP_FAILED);
+	CHECK(mprotect((char *)pdu->mapping + page, page, PROT_NONE) == 0);
+	/* Any read or write past the declared payload reaches the guard page. */
+	pdu->hdr = (struct isns_hdr *)((char *)pdu->mapping + page -
+				      sizeof(*pdu->hdr) - length);
+	memset(pdu->hdr, 0, sizeof(*pdu->hdr) + length);
+	pdu->hdr->length = htons(length);
+	pdu->hdr->transaction = htons(17);
+	return 0;
+}
+
+static void put_tlv(void *buffer, uint32_t tag, uint32_t length, const void *value)
+{
+	struct isns_tlv *tlv = buffer;
+
+	tlv->tag = htonl(tag);
+	tlv->length = htonl(length);
+	if (value)
+		memcpy(tlv->value, value, length);
+}
+
+static int check_short_scn(void)
+{
+	size_t length;
+
+	for (length = 0; length < sizeof(struct isns_tlv); length++) {
+		struct guarded_pdu pdu;
+
+		CHECK(make_pdu(&pdu, length) == 0);
+		CHECK(!print_scn_pdu(pdu.hdr));
+		CHECK(munmap(pdu.mapping, pdu.size) == 0);
+	}
+	return 0;
+}
+
+static int check_scn(const char *scenario)
+{
+	struct guarded_pdu pdu;
+	uint32_t bitmap = htonl(1);
+	bool valid = strcmp(scenario, "scn-valid") == 0;
+	bool delimiter = strcmp(scenario, "scn-delimiter") == 0;
+	bool tail = strcmp(scenario, "scn-tail") == 0;
+	bool unaligned = strcmp(scenario, "scn-unaligned") == 0;
+	bool oversized = strcmp(scenario, "scn-oversized") == 0;
+	size_t length = valid ? 24 : delimiter ? 20 : tail ? 16 : unaligned ? 9 : 12;
+	char *name;
+
+	CHECK(valid || delimiter || tail || unaligned || oversized ||
+	      strcmp(scenario, "scn-truncated") == 0);
+	CHECK(make_pdu(&pdu, length) == 0);
+	if (valid) {
+		put_tlv(pdu.hdr->pdu, ISNS_ATTR_ISCSI_NAME, 4, "new");
+		put_tlv((char *)pdu.hdr->pdu + 12, ISNS_ATTR_ISCSI_SCN_BITMAP, 4, &bitmap);
+	} else if (delimiter) {
+		put_tlv(pdu.hdr->pdu, 0, 0, NULL);
+		put_tlv((char *)pdu.hdr->pdu + 8, ISNS_ATTR_ISCSI_NAME, 4, "new");
+	} else if (tail) {
+		put_tlv(pdu.hdr->pdu, ISNS_ATTR_ISCSI_NAME, 4, "oldX");
+	} else {
+		put_tlv(pdu.hdr->pdu, ISNS_ATTR_ISCSI_NAME,
+			oversized ? UINT32_MAX : unaligned ? 1 : 8, NULL);
+	}
+	name = print_scn_pdu(pdu.hdr);
+	if (valid || delimiter) {
+		CHECK(name && strcmp(name, "new") == 0);
+	} else {
+		CHECK(!name);
+		if (tail)
+			CHECK(memcmp((char *)pdu.hdr->pdu + 8, "oldX", 4) == 0);
+	}
+	CHECK(munmap(pdu.mapping, pdu.size) == 0);
+	return 0;
+}
+
+static int check_query(size_t length, const char *scenario)
+{
+	struct guarded_pdu pdu;
+	struct isns_initiator *original, *current;
+	struct isns_qry_mgmt *mgmt;
+	uint32_t node_type = htonl(ISNS_NODE_INITIATOR);
+	bool valid = strcmp(scenario, "query-valid") == 0;
+	bool tail = strcmp(scenario, "query-tail") == 0;
+
+	CHECK(make_pdu(&pdu, length) == 0);
+	INIT_LIST_HEAD(&qry_list);
+	INIT_LIST_HEAD(&probe_target.isns_head);
+	snprintf(probe_target.name, sizeof(probe_target.name), "target");
+	original = calloc(1, sizeof(*original));
+	mgmt = calloc(1, sizeof(*mgmt));
+	CHECK(original && mgmt);
+	snprintf(original->name, sizeof(original->name), "old");
+	list_add_tail(&original->ilist, &probe_target.isns_head);
+	mgmt->transaction = 17;
+	snprintf(mgmt->name, sizeof(mgmt->name), "%s", probe_target.name);
+	list_add_tail(&mgmt->qlist, &qry_list);
+	if (valid || tail) {
+		put_tlv((char *)pdu.hdr->pdu + 4, ISNS_ATTR_ISCSI_NAME, 4, "new");
+		put_tlv((char *)pdu.hdr->pdu + 16, ISNS_ATTR_ISCSI_NODE_TYPE, 4, &node_type);
+	} else if (strcmp(scenario, "query-oversized") == 0) {
+		put_tlv((char *)pdu.hdr->pdu + 4, ISNS_ATTR_ISCSI_NAME, UINT32_MAX, NULL);
+	} else if (strcmp(scenario, "query-error") == 0) {
+		pdu.hdr->pdu[0] = htonl(3);
+	}
+	qry_rsp_handle(pdu.hdr);
+	CHECK(list_empty(&qry_list));
+	CHECK(!list_empty(&probe_target.isns_head));
+	current = list_entry(probe_target.isns_head.q_forw, struct isns_initiator, ilist);
+	CHECK(current->ilist.q_forw == &probe_target.isns_head);
+	CHECK(strcmp(current->name, valid ? "new" : "old") == 0);
+	if (!valid)
+		CHECK(current == original);
+	free_all_acl(&probe_target);
+	CHECK(munmap(pdu.mapping, pdu.size) == 0);
+	return 0;
+}
+
+int main(int argc, char **argv)
+{
+	const char *scenario;
+	size_t length;
+
+	CHECK(argc == 2);
+	scenario = argv[1];
+	if (strcmp(scenario, "scn-short") == 0) {
+		CHECK(check_short_scn() == 0);
+	} else if (strncmp(scenario, "scn-", 4) == 0) {
+		CHECK(check_scn(scenario) == 0);
+	} else if (strcmp(scenario, "query-short") == 0) {
+		for (length = 0; length < 4; length++)
+			CHECK(check_query(length, scenario) == 0);
+	} else if (strcmp(scenario, "query-short-tlv") == 0) {
+		for (length = 5; length < 12; length++)
+			CHECK(check_query(length, scenario) == 0);
+	} else if (strcmp(scenario, "query-valid") == 0) {
+		CHECK(check_query(28, scenario) == 0);
+	} else if (strcmp(scenario, "query-tail") == 0) {
+		CHECK(check_query(32, scenario) == 0);
+	} else if (strcmp(scenario, "query-oversized") == 0) {
+		CHECK(check_query(12, scenario) == 0);
+	} else if (strcmp(scenario, "query-error") == 0) {
+		CHECK(check_query(4, scenario) == 0);
+	} else {
+		CHECK(0);
+	}
+	puts("ok");
+	return 0;
+}