From 4883fd790e1a9bd95ff11a976abb656e38ef15c1 Mon Sep 17 00:00:00 2001 From: Gleb Chesnokov Date: Wed, 21 Oct 2026 13:28:37 +0300 Subject: [PATCH] iscsi-scstd: Bound source reads in the private string-copy helper Checkpatch reports STRLCPY at the daemon copy sites: Prefer strscpy over strlcpy The private strlcpy() implementation calls strlen(src) even when the caller only needs a short prefix. Manual inspection of that warning shows an unbounded source scan, including for a zero-capacity destination. Replace it with iscsi_strscpy(), which examines at most size source bytes, returns -E2BIG on truncation and terminates every nonempty destination. Use a private name instead of defining a replacement libc strlcpy symbol. All thirty callers discard the previous full-source-length result; their bounded, NUL-terminated output remains the same for valid string inputs. Add isolated Ginkgo coverage of the production userspace helper through the existing C-probe mechanism. Guard both buffers with inaccessible pages and cover zero capacity, empty strings, fitting strings, exact fits, one-byte destinations and nonterminated sources at the copy limit. No kernel code is compiled into this userspace probe. --- iscsi-scst/usr/config.c | 6 +- iscsi-scst/usr/ctldev.c | 43 ++++----- iscsi-scst/usr/event.c | 6 +- iscsi-scst/usr/iscsi_adm.c | 12 +-- iscsi-scst/usr/param.c | 28 +++--- iscsi-scst/usr/param.h | 2 +- iscsi-scst/usr/target.c | 2 +- tests/ginkgo/README.md | 4 + tests/ginkgo/internal/param/param_test.go | 78 ++++++++++++++++ .../internal/param/testdata/string_probe.c | 90 +++++++++++++++++++ 10 files changed, 224 insertions(+), 47 deletions(-) create mode 100644 tests/ginkgo/internal/param/param_test.go create mode 100644 tests/ginkgo/internal/param/testdata/string_probe.c diff --git a/iscsi-scst/usr/config.c b/iscsi-scst/usr/config.c index 3cb97d4c5..25fb9538f 100644 --- a/iscsi-scst/usr/config.c +++ b/iscsi-scst/usr/config.c @@ -147,7 +147,7 @@ int iscsi_attr_create(int attr_size, struct __qelem *attrs_list, goto add; if (sysfs_name_tmpl != NULL) { - strlcpy(attr->sysfs_name, sysfs_name_tmpl, sizeof(attr->sysfs_name)); + iscsi_strscpy(attr->sysfs_name, sysfs_name_tmpl, sizeof(attr->sysfs_name)); if (iscsi_attr_lookup_by_sysfs_name(attrs_list, attr->sysfs_name) == NULL) goto add; } @@ -390,7 +390,7 @@ int config_account_query(u32 tid, int dir, const char *name, char *pass) if (!(user = account_lookup_by_name(tid, dir, name))) return -ENOENT; - strlcpy(pass, ISCSI_USER_PASS(user), ISCSI_NAME_LEN); + iscsi_strscpy(pass, ISCSI_USER_PASS(user), ISCSI_NAME_LEN); return 0; } @@ -418,7 +418,7 @@ int config_account_list(u32 tid, int dir, u32 *cnt, u32 *overflow, list_for_each_entry(user, list, ulist) { if (buf_sz >= ISCSI_NAME_LEN) { - strlcpy(buf, ISCSI_USER_NAME(user), ISCSI_NAME_LEN); + iscsi_strscpy(buf, ISCSI_USER_NAME(user), ISCSI_NAME_LEN); buf_sz -= ISCSI_NAME_LEN; buf += ISCSI_NAME_LEN; *cnt += 1; diff --git a/iscsi-scst/usr/ctldev.c b/iscsi-scst/usr/ctldev.c index 49efe5677..2ef7cb3e3 100644 --- a/iscsi-scst/usr/ctldev.c +++ b/iscsi-scst/usr/ctldev.c @@ -74,7 +74,7 @@ int kernel_target_create(struct target *target, u32 *tid, u32 cookie) struct iscsi_kern_attr *kern_attrs; memset(&info, 0, sizeof(info)); - strlcpy(info.name, target->name, sizeof(info.name)); + iscsi_strscpy(info.name, target->name, sizeof(info.name)); info.tid = (tid != NULL) ? *tid : 0; info.cookie = cookie; @@ -113,52 +113,52 @@ int kernel_target_create(struct target *target, u32 *tid, u32 cookie) i = 0; kern_attrs[i].mode = 0644; - strlcpy(kern_attrs[i].name, ISCSI_PER_PORTAL_ACL_ATTR_NAME, - sizeof(ISCSI_PER_PORTAL_ACL_ATTR_NAME)); + iscsi_strscpy(kern_attrs[i].name, ISCSI_PER_PORTAL_ACL_ATTR_NAME, + sizeof(ISCSI_PER_PORTAL_ACL_ATTR_NAME)); i++; kern_attrs[i].mode = 0644; - strlcpy(kern_attrs[i].name, ISCSI_TARGET_REDIRECTION_ATTR_NAME, - sizeof(ISCSI_TARGET_REDIRECTION_ATTR_NAME)); + iscsi_strscpy(kern_attrs[i].name, ISCSI_TARGET_REDIRECTION_ATTR_NAME, + sizeof(ISCSI_TARGET_REDIRECTION_ATTR_NAME)); i++; kern_attrs[i].mode = 0644; - strlcpy(kern_attrs[i].name, ISCSI_TARGET_ALIAS_ATTR_NAME, - sizeof(ISCSI_TARGET_ALIAS_ATTR_NAME)); + iscsi_strscpy(kern_attrs[i].name, ISCSI_TARGET_ALIAS_ATTR_NAME, + sizeof(ISCSI_TARGET_ALIAS_ATTR_NAME)); i++; for (j = 0; j < session_key_last; j++) { if (!session_keys[j].show_in_sysfs) continue; kern_attrs[i].mode = 0644; - strlcpy(kern_attrs[i].name, session_keys[j].name, - sizeof(kern_attrs[i].name)); + iscsi_strscpy(kern_attrs[i].name, session_keys[j].name, + sizeof(kern_attrs[i].name)); i++; } for (j = 0; j < target_key_last; j++) { if (!target_keys[j].show_in_sysfs) continue; kern_attrs[i].mode = 0644; - strlcpy(kern_attrs[i].name, target_keys[j].name, - sizeof(kern_attrs[i].name)); + iscsi_strscpy(kern_attrs[i].name, target_keys[j].name, + sizeof(kern_attrs[i].name)); i++; } list_for_each_entry(user, &target->target_in_accounts, ulist) { kern_attrs[i].mode = user->sysfs_mode; - strlcpy(kern_attrs[i].name, user->sysfs_name, - sizeof(kern_attrs[i].name)); + iscsi_strscpy(kern_attrs[i].name, user->sysfs_name, + sizeof(kern_attrs[i].name)); i++; } list_for_each_entry(user, &target->target_out_accounts, ulist) { kern_attrs[i].mode = user->sysfs_mode; - strlcpy(kern_attrs[i].name, user->sysfs_name, - sizeof(kern_attrs[i].name)); + iscsi_strscpy(kern_attrs[i].name, user->sysfs_name, + sizeof(kern_attrs[i].name)); i++; } list_for_each_entry(portal, &target->allowed_portals, ulist) { kern_attrs[i].mode = portal->sysfs_mode; - strlcpy(kern_attrs[i].name, portal->sysfs_name, - sizeof(kern_attrs[i].name)); + iscsi_strscpy(kern_attrs[i].name, portal->sysfs_name, + sizeof(kern_attrs[i].name)); i++; } @@ -213,7 +213,7 @@ int kernel_attr_add(struct target *target, const char *name, u32 mode, info.cookie = cookie; info.attr.mode = mode; - strlcpy(info.attr.name, name, sizeof(info.attr.name)); + iscsi_strscpy(info.attr.name, name, sizeof(info.attr.name)); res = ioctl(ctrl_fd, ISCSI_ATTR_ADD, &info); if (res < 0) @@ -232,7 +232,7 @@ int kernel_attr_del(struct target *target, const char *name, u32 cookie) info.tid = target->tid; info.cookie = cookie; - strlcpy(info.attr.name, name, sizeof(info.attr.name)); + iscsi_strscpy(info.attr.name, name, sizeof(info.attr.name)); res = ioctl(ctrl_fd, ISCSI_ATTR_DEL, &info); if (res < 0) @@ -259,7 +259,8 @@ int kernel_initiator_allowed(u32 tid, const char *full_initiator_name) memset(&info, 0, sizeof(info)); info.tid = tid; - strlcpy(info.full_initiator_name, full_initiator_name, sizeof(info.full_initiator_name)); + iscsi_strscpy(info.full_initiator_name, full_initiator_name, + sizeof(info.full_initiator_name)); if ((err = ioctl(ctrl_fd, ISCSI_INITIATOR_ALLOWED, &info)) < 0) { err = -errno; @@ -375,7 +376,7 @@ int kernel_session_create(struct connection *conn) info.tid = conn->tid; info.sid = conn->sess->sid.id64; info.exp_cmd_sn = conn->exp_cmd_sn; - strlcpy(info.initiator_name, conn->sess->initiator, sizeof(info.initiator_name)); + iscsi_strscpy(info.initiator_name, conn->sess->initiator, sizeof(info.initiator_name)); iscsi_make_full_initiator_name(target->per_portal_acl, diff --git a/iscsi-scst/usr/event.c b/iscsi-scst/usr/event.c index b47880352..769505987 100644 --- a/iscsi-scst/usr/event.c +++ b/iscsi-scst/usr/event.c @@ -131,7 +131,7 @@ static int send_mgmt_cmd_res(u32 tid, u32 cookie, u32 req_cmd, int result, cinfo.result = result; if (res_str != NULL) - strlcpy(cinfo.value, res_str, sizeof(cinfo.value)); + iscsi_strscpy(cinfo.value, res_str, sizeof(cinfo.value)); log_debug(1, "Sending result %d (cookie %d)", result, cookie); @@ -751,7 +751,7 @@ static int handle_target_redirect(struct target *target, char *p) t = addr+1; else t = addr; - strlcpy(tmp, t, strchrnul(t, ']')-t+1); + iscsi_strscpy(tmp, t, strchrnul(t, ']') - t + 1); if (inet_pton(AF_INET6, tmp, &ia) != 1) { log_error("Invalid addr %s", addr); res = -EINVAL; @@ -1096,7 +1096,7 @@ static int handle_e_set_attr_value(int fd, const struct iscsi_kern_event *event) goto out_free_server; } else if (strcasecmp(ISCSI_ISNS_ENTITY_ATTR_NAME, pp) == 0) { pp = config_sep_string(&p); - strlcpy(isns_entity_target_name, pp, sizeof(isns_entity_target_name)); + iscsi_strscpy(isns_entity_target_name, pp, sizeof(isns_entity_target_name)); } else if (strcasecmp(ISCSI_LINK_LOCAL_ATTR_NAME, pp) == 0) { if (target != NULL) { log_error("Not NULL target %s for global attribute %s", diff --git a/iscsi-scst/usr/iscsi_adm.c b/iscsi-scst/usr/iscsi_adm.c index eb3683317..de5248bd7 100644 --- a/iscsi-scst/usr/iscsi_adm.c +++ b/iscsi-scst/usr/iscsi_adm.c @@ -261,7 +261,7 @@ static void show_iscsi_params(int type, struct iscsi_param *param) for (i = 0; i < nr; i++) { memset(buf, 0, sizeof(buf)); - strlcpy(buf, keys[i].name, sizeof(buf)); + iscsi_strscpy(buf, keys[i].name, sizeof(buf)); len = strlen(buf); p = buf + len; *p++ = '='; @@ -348,7 +348,7 @@ static int trgt_handle(int op, u32 set, u32 tid, char *params) goto out; } req.rcmnd = C_TRGT_NEW; - strlcpy(req.u.trgt.name, p, sizeof(req.u.trgt.name)); + iscsi_strscpy(req.u.trgt.name, p, sizeof(req.u.trgt.name)); break; } case OP_DELETE: @@ -464,7 +464,7 @@ static int user_handle_show_user(struct iscsi_adm_req *req, char *user) int err; req->rcmnd = C_ACCT_SHOW; - strlcpy(req->u.acnt.u.user.name, user, sizeof(req->u.acnt.u.user.name)); + iscsi_strscpy(req->u.acnt.u.user.name, user, sizeof(req->u.acnt.u.user.name)); err = iscsid_request(req, NULL, 0); if (!err) @@ -547,8 +547,8 @@ static int user_handle_new(struct iscsi_adm_req *req, char *user, char *pass) req->rcmnd = C_ACCT_NEW; - strlcpy(req->u.acnt.u.user.name, user, sizeof(req->u.acnt.u.user.name)); - strlcpy(req->u.acnt.u.user.pass, pass, sizeof(req->u.acnt.u.user.pass)); + iscsi_strscpy(req->u.acnt.u.user.name, user, sizeof(req->u.acnt.u.user.name)); + iscsi_strscpy(req->u.acnt.u.user.pass, pass, sizeof(req->u.acnt.u.user.pass)); return iscsid_request(req, NULL, 0); } @@ -565,7 +565,7 @@ static int user_handle_del(struct iscsi_adm_req *req, char *user, char *pass) req->rcmnd = C_ACCT_DEL; - strlcpy(req->u.acnt.u.user.name, user, sizeof(req->u.acnt.u.user.name)); + iscsi_strscpy(req->u.acnt.u.user.name, user, sizeof(req->u.acnt.u.user.name)); return iscsid_request(req, NULL, 0); } diff --git a/iscsi-scst/usr/param.c b/iscsi-scst/usr/param.c index 0d3ef2aa7..a1006c452 100644 --- a/iscsi-scst/usr/param.c +++ b/iscsi-scst/usr/param.c @@ -22,18 +22,22 @@ #include "iscsid.h" -/* Taken from Linux kernel sources */ -size_t strlcpy(char *dest, const char *src, size_t size) +/* Copy a string without reading beyond size bytes; report truncation. */ +ssize_t iscsi_strscpy(char *dest, const char *src, size_t size) { - size_t ret = strlen(src); + size_t len; - if (size) { - size_t len = (ret >= size) ? size - 1 : ret; + if (!size) + return -E2BIG; - memcpy(dest, src, len); - dest[len] = '\0'; + len = strnlen(src, size); + if (len == size) { + memcpy(dest, src, size - 1); + dest[size - 1] = '\0'; + return -E2BIG; } - return ret; + memcpy(dest, src, len + 1); + return len; } int params_index_by_name(const char *name, const struct iscsi_key *keys) @@ -102,10 +106,10 @@ static int bool_val_to_str(unsigned int val, char *str, int len) switch (val) { case 0: - strlcpy(str, "No", len); + iscsi_strscpy(str, "No", len); break; case 1: - strlcpy(str, "Yes", len); + iscsi_strscpy(str, "Yes", len); break; default: err = -EINVAL; @@ -247,9 +251,9 @@ static int digest_set_val(struct iscsi_param *param, int idx, unsigned int *val) static int marker_val_to_str(unsigned int val, char *str, int len) { if (val == 0) - strlcpy(str, "Irrelevant", len); + iscsi_strscpy(str, "Irrelevant", len); else - strlcpy(str, "Reject", len); + iscsi_strscpy(str, "Reject", len); return 0; } diff --git a/iscsi-scst/usr/param.h b/iscsi-scst/usr/param.h index e936c2593..1992e5cdb 100644 --- a/iscsi-scst/usr/param.h +++ b/iscsi-scst/usr/param.h @@ -57,7 +57,7 @@ extern struct iscsi_key session_keys[]; extern struct iscsi_key target_keys[]; extern struct iscsi_key user_keys[]; -extern size_t strlcpy(char *dest, const char *src, size_t size); +ssize_t iscsi_strscpy(char *dest, const char *src, size_t size); extern void params_set_defaults(unsigned int *, const struct iscsi_key *); extern int params_index_by_name(const char *, const struct iscsi_key *); diff --git a/iscsi-scst/usr/target.c b/iscsi-scst/usr/target.c index c82708101..b8c8c73dc 100644 --- a/iscsi-scst/usr/target.c +++ b/iscsi-scst/usr/target.c @@ -422,7 +422,7 @@ int target_create(const char *name, struct target **out_target) } memset(target, 0, sizeof(*target)); - strlcpy(target->name, name, sizeof(target->name)); + iscsi_strscpy(target->name, name, sizeof(target->name)); params_set_defaults(target->target_params, target_keys); params_set_defaults(target->session_params, session_keys); diff --git a/tests/ginkgo/README.md b/tests/ginkgo/README.md index 95d626696..3d6b8d279 100644 --- a/tests/ginkgo/README.md +++ b/tests/ginkgo/README.md @@ -137,6 +137,10 @@ 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. +The isolated `internal/param` suite checks the daemon string-copy helper +with guard pages, covering empty, fitting and truncated strings and zero +capacity. It runs through `ginkgo-check` with `cc`, without a running daemon. + ## SCSI and I/O Install Python 3, util-linux and sg3_utils on the command host: the target for diff --git a/tests/ginkgo/internal/param/param_test.go b/tests/ginkgo/internal/param/param_test.go new file mode 100644 index 000000000..afd6c098c --- /dev/null +++ b/tests/ginkgo/internal/param/param_test.go @@ -0,0 +1,78 @@ +// SPDX-License-Identifier: GPL-2.0-only +package param + +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/string_probe.c +var stringProbe string + +func TestStringCopy(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "SCST daemon string copies") +} + +var _ = Describe("Bounded daemon string copies", Label("unit", "userspace", "param"), func() { + var probe string + var client *admin.Client + + BeforeEach(func(ctx SpecContext) { + client = &admin.Client{Log: GinkgoWriter} + dir, err := os.MkdirTemp("", "scst-param-probe-") + Expect(err).NotTo(HaveOccurred()) + DeferCleanup(func() { + if client.Blocked() != nil { + AddReportEntry("preserved string-copy 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, "string_probe.c") + Expect(os.WriteFile(input, []byte(stringProbe), 0600)).To(Succeed()) + probe = filepath.Join(dir, "string-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("bounds source reads and destination writes", 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("zero capacity touches neither pointer", "zero"), + Entry("an empty source writes only its terminator", "empty"), + Entry("a fitting string leaves the tail untouched", "fits"), + Entry("an exact fit retains its terminator", "exact-fit"), + Entry("a one-byte destination is terminated", "one-byte"), + Entry("a full source stops before the guard page", "truncated"), + ) +}) diff --git a/tests/ginkgo/internal/param/testdata/string_probe.c b/tests/ginkgo/internal/param/testdata/string_probe.c new file mode 100644 index 000000000..bc0941f54 --- /dev/null +++ b/tests/ginkgo/internal/param/testdata/string_probe.c @@ -0,0 +1,90 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#include +#include +#include +#include +#include +#include + +/* Compile the production helper without starting the daemon. */ +#include "param.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) + +int main(int argc, char **argv) +{ + char *source_mapping, *destination_mapping, *source, *destination; + const char *expected = NULL; + ssize_t result; + size_t capacity, copied, i; + long page = sysconf(_SC_PAGESIZE); + bool truncated; + + CHECK(argc == 2 && page > 16); + truncated = strcmp(argv[1], "truncated") == 0 || + strcmp(argv[1], "one-byte") == 0; + if (strcmp(argv[1], "zero") == 0) { + capacity = 0; + } else if (strcmp(argv[1], "one-byte") == 0) { + capacity = 1; + } else if (strcmp(argv[1], "empty") == 0) { + capacity = 8; + expected = ""; + } else if (strcmp(argv[1], "fits") == 0) { + capacity = 8; + expected = "abc"; + } else if (strcmp(argv[1], "exact-fit") == 0) { + capacity = 4; + expected = "abc"; + } else { + CHECK(truncated); + capacity = 8; + } + + source_mapping = mmap(NULL, 2 * page, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + destination_mapping = mmap(NULL, 2 * page, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + CHECK(source_mapping != MAP_FAILED && destination_mapping != MAP_FAILED); + CHECK(mprotect(source_mapping + page, page, PROT_NONE) == 0); + CHECK(mprotect(destination_mapping + page, page, PROT_NONE) == 0); + source = source_mapping + page - capacity; + destination = destination_mapping + page - capacity; + destination[-1] = 0x5a; + if (capacity) { + memset(source, 'x', capacity); + memset(destination, 0x5a, capacity); + if (expected) + memcpy(source, expected, strlen(expected) + 1); + } + + /* A source without a NUL ends exactly at the inaccessible page. */ + result = iscsi_strscpy(destination, source, capacity); + CHECK(destination[-1] == 0x5a); + if (!capacity) { + CHECK(result == -E2BIG); + } else if (truncated) { + CHECK(result == -E2BIG); + for (i = 0; i < capacity - 1; i++) + CHECK(destination[i] == 'x'); + CHECK(destination[capacity - 1] == '\0'); + } else { + copied = strlen(expected) + 1; + CHECK(result == (ssize_t)copied - 1); + CHECK(memcmp(destination, expected, copied) == 0); + for (i = copied; i < capacity; i++) + CHECK(destination[i] == 0x5a); + } + CHECK(munmap(source_mapping, 2 * page) == 0); + CHECK(munmap(destination_mapping, 2 * page) == 0); + puts("ok"); + return 0; +}