From e2243f45f17611b54672a7aa1f09dae8805237da Mon Sep 17 00:00:00 2001 From: Gleb Chesnokov Date: Wed, 21 Oct 2026 09:51:32 +0300 Subject: [PATCH] iscsi-scstd: Log an invalid portal before freeing its storage Manual review found a use-after-free in internal_portal validation; no saved analyzer diagnostic directly reports this aliasing case. portal points inside the allocation owned by portals, but the error branch frees portals before passing portal to the logging function. Keep the allocation alive until the invalid address has been logged. Preserve the existing EINVAL response and cleanup path. Add a Ginkgo management regression through scstadmin. Reject a list starting with an invalid token, check that the daemon log contains the intact token, and verify daemon liveness and the unchanged setting. --- iscsi-scst/usr/event.c | 2 +- tests/ginkgo/iscsi/portal_validation_test.go | 50 ++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 tests/ginkgo/iscsi/portal_validation_test.go diff --git a/iscsi-scst/usr/event.c b/iscsi-scst/usr/event.c index b2178ae08..b47880352 100644 --- a/iscsi-scst/usr/event.c +++ b/iscsi-scst/usr/event.c @@ -1140,8 +1140,8 @@ static int handle_e_set_attr_value(int fd, const struct iscsi_kern_event *event) portal = strtok(portals, " "); while (portal != NULL) { if (!is_addr(portal)) { - free(portals); log_error("Invalid address supplied %s", portal); + free(portals); res = -EINVAL; goto out_free; } diff --git a/tests/ginkgo/iscsi/portal_validation_test.go b/tests/ginkgo/iscsi/portal_validation_test.go new file mode 100644 index 000000000..a2fbc21a6 --- /dev/null +++ b/tests/ginkgo/iscsi/portal_validation_test.go @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: GPL-2.0-only +package iscsi + +import ( + "os" + "path/filepath" + "strings" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "scst-component-tests/internal/testcfg" +) + +var _ = It("rejects an invalid portal list and logs the intact offending token", Label("basic", "iscsi", "negative"), func(ctx SpecContext) { + attrs := must(cli.Attributes(ctx, "-list_drv_attr", "iscsi")) + before, ok := attrs["internal_portal"] + Expect(ok).To(BeTrue()) + restoreValue := strings.Join(before.Values, " ") + // scstadmin displays an empty attribute as . + if restoreValue == "" { + restoreValue = "" + } + DeferCleanup(func(ctx SpecContext) { + if cli.Blocked() != nil || daemon.Running() != nil { + AbortSuite("cannot verify or restore the internal portal setting") + } + do(ctx, "-set_drv_attr", "iscsi", "-attributes", "internal_portal="+restoreValue) + }, NodeTimeout(testcfg.Duration(cfg.CleanupTimeout))) + + logPath := filepath.Join(artifactDir, "iscsi-scstd.log") + logStart := len(must(os.ReadFile(logPath))) + bad := "invalid-portal-" + runID + // A second token selects the list-validation path. The invalid first + // token is at the start of the allocation that used to be freed before + // logging, making allocator metadata corruption observable in the log. + r, err := cli.Execute(ctx, "-set_drv_attr", "iscsi", "-attributes", "internal_portal="+bad+" 127.0.0.1") + Expect(err).NotTo(HaveOccurred(), r.String()) + Expect(r.ExitCode != 0 || r.ReportedError()).To(BeTrue(), r.String()) + Expect(r.Stderr).To(ContainSubstring("(EINVAL)"), r.String()) + Eventually(func() string { + Expect(daemon.Running()).To(Succeed()) + log := must(os.ReadFile(logPath)) + Expect(len(log)).To(BeNumerically(">=", logStart)) + return string(log[logStart:]) + }, testcfg.Duration(cfg.SettleTimeout), testcfg.Duration(cfg.PollInterval)).WithContext(ctx). + Should(ContainSubstring("Invalid address supplied " + bad)) + after := must(cli.Attributes(ctx, "-list_drv_attr", "iscsi")) + Expect(after["internal_portal"]).To(Equal(before)) +})