From nobody Wed Apr 05 20:42:07 2023 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4PsGlN1yqpz447f5; Wed, 5 Apr 2023 20:42:08 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4PsGlM6xLWz4T92; Wed, 5 Apr 2023 20:42:07 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1680727328; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=azJ6ZsMZbK+pkyYIxrE9XmF9W+pUe8wT/J5Rd6kWrbg=; b=eBbn04lw3RGFablD5vPMAnTvRWsf7eVpo4Tg45FVaPBmUcwD+BjvajTbCcxTPPkuzmjPC9 FJ2L1b2i1D9x/4rJY0xQWLDHsLlwUjyiZHViCf/VIBZ4l4LZv0XVwPownChApABDmt+/o3 TmCdnMvzuAoNrKAUp1XWT/z6yPfE64QiSfgWOiVYI6Wmf/Cl2As8GQnT1xKJ8IDapTXmsU V8OsLTLiPZhqeW34yZZeTPpVf3VohNvl8vmVEKR6gRtSGKUUzwIBflo+/UbtCdE52IdHmt AuL1MNtj/MSYwLw25sNIm6hd5otf5VRUGcmgD/FPyVEs3xxf8hHdcda8i7PouQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1680727328; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=azJ6ZsMZbK+pkyYIxrE9XmF9W+pUe8wT/J5Rd6kWrbg=; b=KlhhWbYTdrlxgDoyXM9iYhGs4i/O7SgYEjoQ6nnqBhAlFOArqpRcVQMspiTxCsQg9hEDiG ecqsw2BMzsicXyWDdWnQfRv2U+DlYmkzUvlCZFYZec1mrYiWrMO6WyP2bw2Mh37VKSkw1l ZvD2ACDE8gjxdYaYgpWrVEAFMpNRxqMzQl0wJoapxE4jFsdBiPXybVFAFh2EQGE4kMRNKC fcRhFppJC+JGTjFVB68/L2BbxHDEng7xq2kAXr7TQDK0/540Cjq+oVrGBYi129nEP+hJjA COYadX2GHHQInmQaZaNSSqFlNYhraP2bmzMS9GpMQ/yiAR8uPSuRq5M0tQ5R5Q== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1680727328; a=rsa-sha256; cv=none; b=h3FAohn5iGi4eH32fYWfsbD1fWA7VpA124XvL0pFSTFxUU1CrYCodVILeqpYSHULm9n0e2 +SFmrUNpQlR9ya2K9t7hCaB76oY7WofGdTQDDITHXkfMQwXBdxZfq1ptDOaHwLoLtVOju8 vqFv8tV9DNmrnMtYvm9faHA3LzKLT/t2UZszUJc+efLCQNgPMqbe8nPu63RDNYElklzkWu dfBPaQhiSlcNpJIL+rxoGim0/8LiSwX/MuavH9NJhxatut+dVvaEoRUvrJLKrhZ9knqpfp IQnJGdxdkGTQcZJWNHzwbGjOHIXpaROFg9y85Y8XaFEGhN9YbJ4MZoyFGp8R0w== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4PsGlM60sxzcsp; Wed, 5 Apr 2023 20:42:07 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 335Kg7AL006822; Wed, 5 Apr 2023 20:42:07 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 335Kg7TL006807; Wed, 5 Apr 2023 20:42:07 GMT (envelope-from git) Date: Wed, 5 Apr 2023 20:42:07 GMT Message-Id: <202304052042.335Kg7TL006807@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Vincenzo Maffione Subject: git: b74063f03a83 - stable/13 - netmap: fix copyin/copyout of nmreq options list List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: vmaffione X-Git-Repository: src X-Git-Refname: refs/heads/stable/13 X-Git-Reftype: branch X-Git-Commit: b74063f03a834e9f22fb46f8e989a9df19823ff0 Auto-Submitted: auto-generated X-ThisMailContainsUnwantedMimeParts: N The branch stable/13 has been updated by vmaffione: URL: https://cgit.FreeBSD.org/src/commit/?id=b74063f03a834e9f22fb46f8e989a9df19823ff0 commit b74063f03a834e9f22fb46f8e989a9df19823ff0 Author: Vincenzo Maffione AuthorDate: 2023-03-21 23:23:18 +0000 Commit: Vincenzo Maffione CommitDate: 2023-04-05 20:41:55 +0000 netmap: fix copyin/copyout of nmreq options list The previous code unsuccesfully attempted to report a precise error for each option in the user list. Moreover, commit 253b2ec199b broke some ctrl-api-test (see https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=260547). With this patch we bail out as soon as an unrecoverable error is detected and we properly check for copy boundaries. EOPNOTSUPP no longer immediately returns an error, so that any other option in the list may be examined by the caller code and a precise report of the (un)supported options can be returned to the user. With this patch, all ctrl-api-test unit tests pass again. PR: 260547 Submitted by: giuseppe.lettieri@unipi.it Reviewed by: vmaffione MFC after: 14 days (cherry picked from commit e2a431a0ffb6894220bdf5d8fc2ca2d0ca316e85) --- sys/dev/netmap/netmap.c | 69 ++++++++++++++++++++++++---------------- tests/sys/netmap/ctrl-api-test.c | 23 ++++++++++++-- 2 files changed, 63 insertions(+), 29 deletions(-) diff --git a/sys/dev/netmap/netmap.c b/sys/dev/netmap/netmap.c index 8c4481c2e6c3..aedb8c048c81 100644 --- a/sys/dev/netmap/netmap.c +++ b/sys/dev/netmap/netmap.c @@ -3103,7 +3103,7 @@ nmreq_copyin(struct nmreq_header *hdr, int nr_body_is_user) size_t rqsz, optsz, bufsz; int error = 0; char *ker = NULL, *p; - struct nmreq_option **next, *src, **opt_tab; + struct nmreq_option **next, *src, **opt_tab, *opt; uint64_t *ptrs; if (hdr->nr_reserved) { @@ -3154,24 +3154,36 @@ nmreq_copyin(struct nmreq_header *hdr, int nr_body_is_user) *ptrs++ = hdr->nr_body; *ptrs++ = hdr->nr_options; p = (char *)ptrs; + /* overwrite the user pointer with the in-kernel one */ + hdr->nr_body = (uintptr_t)p; + /* prepare the options-list pointers and temporarily terminate + * the in-kernel list, in case we have to jump to out_restore + */ + next = (struct nmreq_option **)&hdr->nr_options; + src = *next; + hdr->nr_options = 0; /* copy the body */ - error = copyin((void *)(uintptr_t)hdr->nr_body, p, rqsz); + error = copyin(*(void **)ker, p, rqsz); if (error) goto out_restore; - /* overwrite the user pointer with the in-kernel one */ - hdr->nr_body = (uintptr_t)p; p += rqsz; /* start of the options table */ opt_tab = (struct nmreq_option **)p; p += sizeof(opt_tab) * NETMAP_REQ_OPT_MAX; /* copy the options */ - next = (struct nmreq_option **)&hdr->nr_options; - src = *next; while (src) { - struct nmreq_option *opt; + struct nmreq_option *nsrc; + if (p - ker + sizeof(uint64_t*) + sizeof(*src) > bufsz) { + error = EMSGSIZE; + /* there might be a loop in the list: don't try to + * copyout the options + */ + hdr->nr_options = 0; + goto out_restore; + } /* copy the option header */ ptrs = (uint64_t *)p; opt = (struct nmreq_option *)(ptrs + 1); @@ -3179,15 +3191,19 @@ nmreq_copyin(struct nmreq_header *hdr, int nr_body_is_user) if (error) goto out_restore; rqsz += sizeof(*src); + p = (char *)(opt + 1); + /* make a copy of the user next pointer */ *ptrs = opt->nro_next; - /* overwrite the user pointer with the in-kernel one */ + /* append the option to the in-kernel list */ *next = opt; - - /* initialize the option as not supported. - * Recognized options will update this field. + /* temporarily teminate the in-kernel list, in case we have to + * jump to out_restore */ - opt->nro_status = EOPNOTSUPP; + nsrc = (struct nmreq_option *)opt->nro_next; + opt->nro_next = 0; + + opt->nro_status = 0; /* check for invalid types */ if (opt->nro_reqtype < 1) { @@ -3195,12 +3211,11 @@ nmreq_copyin(struct nmreq_header *hdr, int nr_body_is_user) nm_prinf("invalid option type: %u", opt->nro_reqtype); opt->nro_status = EINVAL; error = EINVAL; - goto next; + goto out_restore; } if (opt->nro_reqtype >= NETMAP_REQ_OPT_MAX) { - /* opt->nro_status is already EOPNOTSUPP */ - error = EOPNOTSUPP; + /* opt->nro_status will be set to EOPNOTSUPP */ goto next; } @@ -3213,12 +3228,10 @@ nmreq_copyin(struct nmreq_header *hdr, int nr_body_is_user) opt->nro_status = EINVAL; opt_tab[opt->nro_reqtype]->nro_status = EINVAL; error = EINVAL; - goto next; + goto out_restore; } opt_tab[opt->nro_reqtype] = opt; - p = (char *)(opt + 1); - /* copy the option body */ optsz = nmreq_opt_size_by_type(opt->nro_reqtype, opt->nro_size); @@ -3241,18 +3254,20 @@ nmreq_copyin(struct nmreq_header *hdr, int nr_body_is_user) next: /* move to next option */ next = (struct nmreq_option **)&opt->nro_next; - src = *next; + src = nsrc; } - if (error) - nmreq_copyout(hdr, error); - return error; + + /* initialize all the options as not supported. Recognized options + * will update their field. + */ + for (src = (struct nmreq_option *)hdr->nr_options; src; + src = (struct nmreq_option *)src->nro_next) { + src->nro_status = EOPNOTSUPP; + } + return 0; out_restore: - ptrs = (uint64_t *)ker; - hdr->nr_body = *ptrs++; - hdr->nr_options = *ptrs++; - hdr->nr_reserved = 0; - nm_os_free(ker); + nmreq_copyout(hdr, error); out_err: return error; } diff --git a/tests/sys/netmap/ctrl-api-test.c b/tests/sys/netmap/ctrl-api-test.c index cea78141fbe4..67c7bfbf5315 100644 --- a/tests/sys/netmap/ctrl-api-test.c +++ b/tests/sys/netmap/ctrl-api-test.c @@ -984,9 +984,10 @@ infinite_options(struct TestContext *ctx) { struct nmreq_option opt; - printf("Testing infinite list of options on %s\n", ctx->ifname_ext); + printf("Testing infinite list of options on %s (invalid options)\n", ctx->ifname_ext); - opt.nro_reqtype = 1234; + memset(&opt, 0, sizeof(opt)); + opt.nro_reqtype = NETMAP_REQ_OPT_MAX + 1; push_option(&opt, ctx); opt.nro_next = (uintptr_t)&opt; if (port_register_hwall(ctx) >= 0) @@ -995,6 +996,23 @@ infinite_options(struct TestContext *ctx) return (errno == EMSGSIZE ? 0 : -1); } +static int +infinite_options2(struct TestContext *ctx) +{ + struct nmreq_option opt; + + printf("Testing infinite list of options on %s (valid options)\n", ctx->ifname_ext); + + memset(&opt, 0, sizeof(opt)); + opt.nro_reqtype = NETMAP_REQ_OPT_OFFSETS; + push_option(&opt, ctx); + opt.nro_next = (uintptr_t)&opt; + if (port_register_hwall(ctx) >= 0) + return -1; + clear_options(ctx); + return (errno == EINVAL ? 0 : -1); +} + #ifdef CONFIG_NETMAP_EXTMEM int change_param(const char *pname, unsigned long newv, unsigned long *poldv) @@ -1755,6 +1773,7 @@ static struct mytest tests[] = { decltest(vale_polling_enable_disable), decltest(unsupported_option), decltest(infinite_options), + decltest(infinite_options2), #ifdef CONFIG_NETMAP_EXTMEM decltest(extmem_option), decltest(bad_extmem_option),