Date: Tue, 28 Oct 2014 22:33:01 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: =?utf-8?Q?Dag-Erling_Sm=C3=B8rgrav?= <des@des.no> Cc: src-committers@freebsd.org, Bruce Evans <brde@optusnet.com.au>, svn-src-all@freebsd.org, svn-src-head@freebsd.org, Kurt Jaeger <pi@freebsd.org>, Konstantin Belousov <kostikbel@gmail.com> Subject: Re: svn commit: r273734 - head/bin/dd Message-ID: <20141028210830.L1034@besplex.bde.org> In-Reply-To: <86mw8ged20.fsf@nine.des.no> References: <201410271138.s9RBcHrA002447@svn.freebsd.org> <20141027153957.GZ1877@kib.kiev.ua> <20141027155427.GM1492@f10.opsec.eu> <86vbn4egjt.fsf@nine.des.no> <20141028200418.E849@besplex.bde.org> <86mw8ged20.fsf@nine.des.no>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 28 Oct 2014, [utf-8] Dag-Erling Sm=C3=B8rgrav wrote: > Bruce Evans <brde@optusnet.com.au> writes: >> Dag-Erling Sm=C3=B8rgrav <des@des.no> writes: >>> This is a bug on all platforms, and both clang and (recent) gcc >>> should complain about it. That printf() call will print garbage. >> No, this is only a bug on 32-bit arches. The is is SSIZE_MAX. > > If you mean "it only has consequences on 32-bit arches", then I agree - > but it is still a bug to pass an int to %jd. This is machine-dependent. intmax_t may be int. The only requirement on intmax_t is that it can represent any value of any signed integer type. This is possible if the largest integer type is 64 bits (the smallest permitted largest type) and int is also 64 bits (and there are no complications for padding bits). intmax_t can even be signed char if that is wide enough (not in POSIX starting 10-15 years ago, since signed char is now required to be 8 bits. It is weird for intmax_t to have the lowest rank (not counting Bool), but FreeBSD uses this loophole to make it have second highest rank on 64-bit arches. > =C2=A77.19.6.1 > > 9 If a conversion specification is invalid, the behavior is > undefined. 239) If any argument is not the correct type for the > corresponding conversion specification, the behavior is undefined. > > The term "correct type" is not defined anywhere, and is only used in the > descriptions of fprintf() and fwprintf(). Binary compatibility gives strong restrictions, and I think the standard is just as strict. Types must match after the default promotions, since otherwise their sizes and representations may be completely different even when their range is the same. "Correct type" for %jd should meant "the same type after the default promotion as intmax_t after its default promotion", but %jd also works for args of type uintmax_t that are not too large, since then they are required to have the same respresentation as when they are cast to the correct type (intmax_t). Bruce From owner-svn-src-head@FreeBSD.ORG Tue Oct 28 12:00:42 2014 Return-Path: <owner-svn-src-head@FreeBSD.ORG> Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 3D3CE28E; Tue, 28 Oct 2014 12:00:42 +0000 (UTC) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:1900:2254:2068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 25606EF6; Tue, 28 Oct 2014 12:00:42 +0000 (UTC) Received: from svn.freebsd.org ([127.0.1.70]) by svn.freebsd.org (8.14.9/8.14.9) with ESMTP id s9SC0gTp010547; Tue, 28 Oct 2014 12:00:42 GMT (envelope-from hselasky@FreeBSD.org) Received: (from hselasky@localhost) by svn.freebsd.org (8.14.9/8.14.9/Submit) id s9SC0eMm010535; Tue, 28 Oct 2014 12:00:40 GMT (envelope-from hselasky@FreeBSD.org) Message-Id: <201410281200.s9SC0eMm010535@svn.freebsd.org> X-Authentication-Warning: svn.freebsd.org: hselasky set sender to hselasky@FreeBSD.org using -f From: Hans Petter Selasky <hselasky@FreeBSD.org> Date: Tue, 28 Oct 2014 12:00:40 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r273773 - in head/sys: dev/acpi_support dev/acpica dev/asmc kern net netinet netinet/cc X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current <svn-src-head.freebsd.org> List-Unsubscribe: <http://lists.freebsd.org/mailman/options/svn-src-head>, <mailto:svn-src-head-request@freebsd.org?subject=unsubscribe> List-Archive: <http://lists.freebsd.org/pipermail/svn-src-head/>; List-Post: <mailto:svn-src-head@freebsd.org> List-Help: <mailto:svn-src-head-request@freebsd.org?subject=help> List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-head>, <mailto:svn-src-head-request@freebsd.org?subject=subscribe> X-List-Received-Date: Tue, 28 Oct 2014 12:00:42 -0000 Author: hselasky Date: Tue Oct 28 12:00:39 2014 New Revision: 273773 URL: https://svnweb.freebsd.org/changeset/base/273773 Log: The SYSCTL data pointers can come from userspace and must not be directly accessed. Although this will work on some platforms, it can throw an exception if the pointer is invalid and then panic the kernel. Add a missing SYSCTL_IN() of "SCTP_BASE_STATS" structure. MFC after: 3 days Sponsored by: Mellanox Technologies Modified: head/sys/dev/acpi_support/acpi_ibm.c head/sys/dev/acpica/acpi.c head/sys/dev/asmc/asmc.c head/sys/kern/kern_ffclock.c head/sys/net/bpf.c head/sys/netinet/cc/cc.c head/sys/netinet/sctp_sysctl.c head/sys/netinet/siftr.c Modified: head/sys/dev/acpi_support/acpi_ibm.c ============================================================================== --- head/sys/dev/acpi_support/acpi_ibm.c Tue Oct 28 11:20:30 2014 (r273772) +++ head/sys/dev/acpi_support/acpi_ibm.c Tue Oct 28 12:00:39 2014 (r273773) @@ -899,6 +899,7 @@ acpi_ibm_handlerevents_sysctl(SYSCTL_HAN char *cp, *ep; int l, val; unsigned int handler_events; + char temp[128]; ACPI_FUNCTION_TRACE((char *)(uintptr_t)__func__); @@ -920,17 +921,18 @@ acpi_ibm_handlerevents_sysctl(SYSCTL_HAN sbuf_trim(&sb); sbuf_finish(&sb); - - /* Copy out the old values to the user. */ - error = SYSCTL_OUT(req, sbuf_data(&sb), sbuf_len(&sb)); + strlcpy(temp, sbuf_data(&sb), sizeof(temp)); sbuf_delete(&sb); + error = sysctl_handle_string(oidp, temp, sizeof(temp), req); + + /* Check for error or no change */ if (error != 0 || req->newptr == NULL) goto out; /* If the user is setting a string, parse it. */ handler_events = 0; - cp = (char *)req->newptr; + cp = temp; while (*cp) { if (isspace(*cp)) { cp++; Modified: head/sys/dev/acpica/acpi.c ============================================================================== --- head/sys/dev/acpica/acpi.c Tue Oct 28 11:20:30 2014 (r273772) +++ head/sys/dev/acpica/acpi.c Tue Oct 28 12:00:39 2014 (r273773) @@ -3783,6 +3783,7 @@ acpi_debug_sysctl(SYSCTL_HANDLER_ARGS) int error, *dbg; struct debugtag *tag; struct sbuf sb; + char temp[128]; if (sbuf_new(&sb, NULL, 128, SBUF_AUTOEXTEND) == NULL) return (ENOMEM); @@ -3806,16 +3807,16 @@ acpi_debug_sysctl(SYSCTL_HANDLER_ARGS) } sbuf_trim(&sb); sbuf_finish(&sb); - - /* Copy out the old values to the user. */ - error = SYSCTL_OUT(req, sbuf_data(&sb), sbuf_len(&sb)); + strlcpy(temp, sbuf_data(&sb), sizeof(temp)); sbuf_delete(&sb); - /* If the user is setting a string, parse it. */ + error = sysctl_handle_string(oidp, temp, sizeof(temp), req); + + /* Check for error or no change */ if (error == 0 && req->newptr != NULL) { - *dbg = 0; - kern_setenv((char *)oidp->oid_arg1, (char *)req->newptr); - acpi_set_debugging(NULL); + *dbg = 0; + kern_setenv((char *)oidp->oid_arg1, temp); + acpi_set_debugging(NULL); } ACPI_SERIAL_END(acpi); Modified: head/sys/dev/asmc/asmc.c ============================================================================== --- head/sys/dev/asmc/asmc.c Tue Oct 28 11:20:30 2014 (r273772) +++ head/sys/dev/asmc/asmc.c Tue Oct 28 12:00:39 2014 (r273773) @@ -1052,7 +1052,7 @@ asmc_mb_sysctl_fanminspeed(SYSCTL_HANDLE error = sysctl_handle_int(oidp, &v, 0, req); if (error == 0 && req->newptr != NULL) { - unsigned int newspeed = *(unsigned int *)req->newptr; + unsigned int newspeed = v; asmc_fan_setvalue(dev, ASMC_KEY_FANMINSPEED, fan, newspeed); } @@ -1071,7 +1071,7 @@ asmc_mb_sysctl_fanmaxspeed(SYSCTL_HANDLE error = sysctl_handle_int(oidp, &v, 0, req); if (error == 0 && req->newptr != NULL) { - unsigned int newspeed = *(unsigned int *)req->newptr; + unsigned int newspeed = v; asmc_fan_setvalue(dev, ASMC_KEY_FANMAXSPEED, fan, newspeed); } @@ -1090,7 +1090,7 @@ asmc_mb_sysctl_fantargetspeed(SYSCTL_HAN error = sysctl_handle_int(oidp, &v, 0, req); if (error == 0 && req->newptr != NULL) { - unsigned int newspeed = *(unsigned int *)req->newptr; + unsigned int newspeed = v; asmc_fan_setvalue(dev, ASMC_KEY_FANTARGETSPEED, fan, newspeed); } @@ -1283,7 +1283,7 @@ asmc_mb_sysctl_sms_z(SYSCTL_HANDLER_ARGS asmc_sms_read(dev, ASMC_KEY_SMS_Z, &val); v = (int32_t) val; - error = sysctl_handle_int(oidp, &v, sizeof(v), req); + error = sysctl_handle_int(oidp, &v, 0, req); return (error); } @@ -1298,7 +1298,7 @@ asmc_mbp_sysctl_light_left(SYSCTL_HANDLE asmc_key_read(dev, ASMC_KEY_LIGHTLEFT, buf, sizeof buf); v = buf[2]; - error = sysctl_handle_int(oidp, &v, sizeof(v), req); + error = sysctl_handle_int(oidp, &v, 0, req); return (error); } @@ -1313,7 +1313,7 @@ asmc_mbp_sysctl_light_right(SYSCTL_HANDL asmc_key_read(dev, ASMC_KEY_LIGHTRIGHT, buf, sizeof buf); v = buf[2]; - error = sysctl_handle_int(oidp, &v, sizeof(v), req); + error = sysctl_handle_int(oidp, &v, 0, req); return (error); } @@ -1324,19 +1324,19 @@ asmc_mbp_sysctl_light_control(SYSCTL_HAN device_t dev = (device_t) arg1; uint8_t buf[2]; int error; - unsigned int level; - static int32_t v; - - error = sysctl_handle_int(oidp, &v, sizeof(v), req); + static unsigned int level; + int v; + + v = level; + error = sysctl_handle_int(oidp, &v, 0, req); + if (error == 0 && req->newptr != NULL) { - level = *(unsigned int *)req->newptr; - if (level > 255) + if (v < 0 || v > 255) return (EINVAL); - v = level; + level = v; buf[0] = level; buf[1] = 0x00; asmc_key_write(dev, ASMC_KEY_LIGHTVALUE, buf, sizeof buf); } - return (error); } Modified: head/sys/kern/kern_ffclock.c ============================================================================== --- head/sys/kern/kern_ffclock.c Tue Oct 28 11:20:30 2014 (r273772) +++ head/sys/kern/kern_ffclock.c Tue Oct 28 12:00:39 2014 (r273773) @@ -203,26 +203,29 @@ static int sysctl_kern_sysclock_active(SYSCTL_HANDLER_ARGS) { char newclock[MAX_SYSCLOCK_NAME_LEN]; - int clk, error; + int error; + int clk; - if (req->newptr == NULL) { - /* Return the name of the current active sysclock. */ - strlcpy(newclock, sysclocks[sysclock_active], sizeof(newclock)); - error = sysctl_handle_string(oidp, newclock, - sizeof(newclock), req); - } else { - /* Change the active sysclock to the user specified one. */ - error = EINVAL; - for (clk = 0; clk < NUM_SYSCLOCKS; clk++) { - if (strncmp((char *)req->newptr, sysclocks[clk], - strlen(sysclocks[clk])) == 0) { - sysclock_active = clk; - error = 0; - break; - } + /* Return the name of the current active sysclock. */ + strlcpy(newclock, sysclocks[sysclock_active], sizeof(newclock)); + error = sysctl_handle_string(oidp, newclock, sizeof(newclock), req); + + /* Check for error or no change */ + if (error != 0 || req->newptr == NULL) + goto done; + + /* Change the active sysclock to the user specified one: */ + error = EINVAL; + for (clk = 0; clk < NUM_SYSCLOCKS; clk++) { + if (strncmp(newclock, sysclocks[clk], + MAX_SYSCLOCK_NAME_LEN - 1)) { + continue; } + sysclock_active = clk; + error = 0; + break; } - +done: return (error); } Modified: head/sys/net/bpf.c ============================================================================== --- head/sys/net/bpf.c Tue Oct 28 11:20:30 2014 (r273772) +++ head/sys/net/bpf.c Tue Oct 28 12:00:39 2014 (r273773) @@ -2807,7 +2807,8 @@ bpfstats_fill_xbpf(struct xbpf_d *d, str static int bpf_stats_sysctl(SYSCTL_HANDLER_ARGS) { - struct xbpf_d *xbdbuf, *xbd, zerostats; + static const struct xbpf_d zerostats; + struct xbpf_d *xbdbuf, *xbd, tempstats; int index, error; struct bpf_if *bp; struct bpf_d *bd; @@ -2827,11 +2828,13 @@ bpf_stats_sysctl(SYSCTL_HANDLER_ARGS) * as we aren't allowing the user to set the counters currently. */ if (req->newptr != NULL) { - if (req->newlen != sizeof(zerostats)) + if (req->newlen != sizeof(tempstats)) return (EINVAL); - bzero(&zerostats, sizeof(zerostats)); - xbd = req->newptr; - if (bcmp(xbd, &zerostats, sizeof(*xbd)) != 0) + memset(&tempstats, 0, sizeof(tempstats)); + error = SYSCTL_IN(req, &tempstats, sizeof(tempstats)); + if (error) + return (error); + if (bcmp(&tempstats, &zerostats, sizeof(tempstats)) != 0) return (EINVAL); bpf_zero_counters(); return (0); Modified: head/sys/netinet/cc/cc.c ============================================================================== --- head/sys/netinet/cc/cc.c Tue Oct 28 11:20:30 2014 (r273772) +++ head/sys/netinet/cc/cc.c Tue Oct 28 12:00:39 2014 (r273773) @@ -92,36 +92,33 @@ cc_default_algo(SYSCTL_HANDLER_ARGS) { char default_cc[TCP_CA_NAME_MAX]; struct cc_algo *funcs; - int err, found; + int error; - err = found = 0; + /* Get the current default: */ + CC_LIST_RLOCK(); + strlcpy(default_cc, CC_DEFAULT()->name, sizeof(default_cc)); + CC_LIST_RUNLOCK(); - if (req->newptr == NULL) { - /* Just print the current default. */ - CC_LIST_RLOCK(); - strlcpy(default_cc, CC_DEFAULT()->name, TCP_CA_NAME_MAX); - CC_LIST_RUNLOCK(); - err = sysctl_handle_string(oidp, default_cc, 0, req); - } else { - /* Find algo with specified name and set it to default. */ - CC_LIST_RLOCK(); - STAILQ_FOREACH(funcs, &cc_list, entries) { - /* NOTE: "newptr" is not zero terminated */ - if (req->newlen != strnlen(funcs->name, - TCP_CA_NAME_MAX - 1)) - continue; - if (bcmp(req->newptr, funcs->name, req->newlen)) - continue; - found = 1; - V_default_cc_ptr = funcs; - } - CC_LIST_RUNLOCK(); + error = sysctl_handle_string(oidp, default_cc, sizeof(default_cc), req); - if (!found) - err = ESRCH; - } + /* Check for error or no change */ + if (error != 0 || req->newptr == NULL) + goto done; - return (err); + error = ESRCH; + + /* Find algo with specified name and set it to default. */ + CC_LIST_RLOCK(); + STAILQ_FOREACH(funcs, &cc_list, entries) { + if (strncmp(default_cc, funcs->name, sizeof(default_cc))) + continue; + V_default_cc_ptr = funcs; + error = 0; + break; + } + CC_LIST_RUNLOCK(); +done: + return (error); } /* Modified: head/sys/netinet/sctp_sysctl.c ============================================================================== --- head/sys/netinet/sctp_sysctl.c Tue Oct 28 11:20:30 2014 (r273772) +++ head/sys/netinet/sctp_sysctl.c Tue Oct 28 12:00:39 2014 (r273773) @@ -636,17 +636,26 @@ sctp_sysctl_handle_stats(SYSCTL_HANDLER_ int error; #if defined(SMP) && defined(SCTP_USE_PERCPU_STAT) + struct sctpstat sb_temp; + struct sctpstat *sarry; + struct sctpstat sb; int cpu; - struct sctpstat sb, *sarry; - #endif if ((req->newptr != NULL) && (req->newlen != sizeof(struct sctpstat))) { return (EINVAL); } + #if defined(SMP) && defined(SCTP_USE_PERCPU_STAT) - memset(&sb, 0, sizeof(struct sctpstat)); + memset(&sb, 0, sizeof(sb)); + memset(&sb_temp, 0, sizeof(sb_temp)); + + if (req->newptr != NULL) { + error = SYSCTL_IN(req, &sb_temp, sizeof(sb_temp)); + if (error != 0) + return (error); + } for (cpu = 0; cpu < mp_maxid; cpu++) { sarry = &SCTP_BASE_STATS[cpu]; if (sarry->sctps_discontinuitytime.tv_sec > sb.sctps_discontinuitytime.tv_sec) { @@ -774,12 +783,14 @@ sctp_sysctl_handle_stats(SYSCTL_HANDLER_ sb.sctps_send_burst_avoid += sarry->sctps_send_burst_avoid; sb.sctps_send_cwnd_avoid += sarry->sctps_send_cwnd_avoid; sb.sctps_fwdtsn_map_over += sarry->sctps_fwdtsn_map_over; - if (req->newptr != NULL) { - memcpy(sarry, req->newptr, sizeof(struct sctpstat)); - } + if (req->newptr != NULL) + memcpy(sarry, &sb_temp, sizeof(struct sctpstat)); } error = SYSCTL_OUT(req, &sb, sizeof(struct sctpstat)); #else + error = SYSCTL_IN(req, &SCTP_BASE_STATS, sizeof(struct sctpstat)); + if (error) + return (error); error = SYSCTL_OUT(req, &SCTP_BASE_STATS, sizeof(struct sctpstat)); #endif return (error); Modified: head/sys/netinet/siftr.c ============================================================================== --- head/sys/netinet/siftr.c Tue Oct 28 11:20:30 2014 (r273772) +++ head/sys/netinet/siftr.c Tue Oct 28 12:00:39 2014 (r273773) @@ -268,6 +268,7 @@ static unsigned int siftr_pkts_per_log = static unsigned int siftr_generate_hashes = 0; /* static unsigned int siftr_binary_log = 0; */ static char siftr_logfile[PATH_MAX] = "/var/log/siftr.log"; +static char siftr_logfile_shadow[PATH_MAX] = "/var/log/siftr.log"; static u_long siftr_hashmask; STAILQ_HEAD(pkthead, pkt_node) pkt_queue = STAILQ_HEAD_INITIALIZER(pkt_queue); LIST_HEAD(listhead, flow_hash_node) *counter_hash; @@ -299,7 +300,7 @@ SYSCTL_PROC(_net_inet_siftr, OID_AUTO, e "switch siftr module operations on/off"); SYSCTL_PROC(_net_inet_siftr, OID_AUTO, logfile, CTLTYPE_STRING|CTLFLAG_RW, - &siftr_logfile, sizeof(siftr_logfile), &siftr_sysctl_logfile_name_handler, + &siftr_logfile_shadow, sizeof(siftr_logfile_shadow), &siftr_sysctl_logfile_name_handler, "A", "file to save siftr log messages to"); SYSCTL_UINT(_net_inet_siftr, OID_AUTO, ppl, CTLFLAG_RW, @@ -1145,38 +1146,38 @@ siftr_sysctl_logfile_name_handler(SYSCTL struct alq *new_alq; int error; - if (req->newptr == NULL) - goto skip; - - /* If old filename and new filename are different. */ - if (strncmp(siftr_logfile, (char *)req->newptr, PATH_MAX)) { - - error = alq_open(&new_alq, req->newptr, curthread->td_ucred, - SIFTR_LOG_FILE_MODE, SIFTR_ALQ_BUFLEN, 0); + error = sysctl_handle_string(oidp, arg1, arg2, req); - /* Bail if unable to create new alq. */ - if (error) - return (1); + /* Check for error or same filename */ + if (error != 0 || req->newptr == NULL || + strncmp(siftr_logfile, arg1, arg2) == 0) + goto done; + + /* Filname changed */ + error = alq_open(&new_alq, arg1, curthread->td_ucred, + SIFTR_LOG_FILE_MODE, SIFTR_ALQ_BUFLEN, 0); + if (error != 0) + goto done; - /* - * If disabled, siftr_alq == NULL so we simply close - * the alq as we've proved it can be opened. - * If enabled, close the existing alq and switch the old - * for the new. - */ - if (siftr_alq == NULL) - alq_close(new_alq); - else { - alq_close(siftr_alq); - siftr_alq = new_alq; - } + /* + * If disabled, siftr_alq == NULL so we simply close + * the alq as we've proved it can be opened. + * If enabled, close the existing alq and switch the old + * for the new. + */ + if (siftr_alq == NULL) { + alq_close(new_alq); + } else { + alq_close(siftr_alq); + siftr_alq = new_alq; } -skip: - return (sysctl_handle_string(oidp, arg1, arg2, req)); + /* Update filename upon success */ + strlcpy(siftr_logfile, arg1, arg2); +done: + return (error); } - static int siftr_manage_ops(uint8_t action) {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141028210830.L1034>