From owner-svn-src-head@freebsd.org Wed Apr 27 09:35:50 2016 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id BEFD5B1EA4B; Wed, 27 Apr 2016 09:35:50 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by mx1.freebsd.org (Postfix) with ESMTP id 6380119E1; Wed, 27 Apr 2016 09:35:49 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c211-30-172-161.carlnfd1.nsw.optusnet.com.au [211.30.172.161]) by mail104.syd.optusnet.com.au (Postfix) with ESMTPS id 0CDFF427618; Wed, 27 Apr 2016 19:04:56 +1000 (AEST) Date: Wed, 27 Apr 2016 19:04:55 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: "Conrad E. Meyer" cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r298671 - head/sys/geom/part In-Reply-To: <201604262230.u3QMUs7W073468@repo.freebsd.org> Message-ID: <20160427173605.H1272@besplex.bde.org> References: <201604262230.u3QMUs7W073468@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=c+ZWOkJl c=1 sm=1 tr=0 a=tA1DmVjRWuAyocftnDq7bQ==:117 a=tA1DmVjRWuAyocftnDq7bQ==:17 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=tlvk3plgDSp4RpOT_RMA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Apr 2016 09:35:50 -0000 On Tue, 26 Apr 2016, Conrad E. Meyer wrote: > Log: > g_part_bsd64: Check for valid on-disk npartitions value > > This value is u32 on disk, but assigned to an int in memory. After we do the > implicit conversion via assignment, check that the result is at least one[1] > (non-negative[2]). > > 1. The subsequent for-loop iterates from gpt_entries minus one, down, until > reaching zero. A negative or zero initial index results in undefined signed > integer overflow. > 2. It is also used to index into arrays later. > > In practice, we expected non-malicious disks to contain small positive values. It is a common programming error to use unsigned types to contain small positive values. This gives [un]sign extension/overflow bugs like (1) by making it difficult to use subtraction operators. (The loop doesn't actually have bug (1) -- it correctly uses only ints in the loop -- see below near the end. Signed integer overflow mostly doesn't occur either. Only an initial value if INT_MIN would give it for subtracting 1. The first undefined behaviour for a negative initial index is actually for the array indexing in crc32(... d_partitions[basetable->gpt_entries]).) struct g_part_table doesn't have as many instances of this bug as struct disklabel or struct disklabel64, so conversions tend to cause [un]sign extension/overflow bugs like [0]. Using unsigned types just gives wrong values with no overflow if the target type is too small. > Reported by: Coverity > CID: 1223202 > Sponsored by: EMC / Isilon Storage Division > > Modified: > head/sys/geom/part/g_part_bsd64.c > > Modified: head/sys/geom/part/g_part_bsd64.c > ============================================================================== > --- head/sys/geom/part/g_part_bsd64.c Tue Apr 26 22:01:07 2016 (r298670) > +++ head/sys/geom/part/g_part_bsd64.c Tue Apr 26 22:30:54 2016 (r298671) > @@ -509,7 +509,8 @@ g_part_bsd64_read(struct g_part_table *b > > dlp = (struct disklabel64 *)buf; > basetable->gpt_entries = le32toh(dlp->d_npartitions); Overflow still occurs here if the source value actually uses all 32 of its bits (assuming 32-bit ints -- otherwise the analysis is even more complicated. POSIX now requires >= 32-bit ints, and FreeBSD never supported anything else, so this assumption is safe for now, but stating it complicates the analysis). > - if (basetable->gpt_entries > MAXPARTITIONS64) > + if (basetable->gpt_entries > MAXPARTITIONS64 || > + basetable->gpt_entries < 1) > goto invalid_label; This check is still bogus, since it checks for overflow after overflow has caused implementation-defined behaviour. This requires a complicated analysis to justify even for the usual behaviour of 2's complement wrapping. I've never seen any implementation that documuments this. The correct check doesn't need any complicated analysis. It just checks the source value before clobbering it by assigning it to a possibly-different type. It is still not so easy to know the correct type to use, since the source value is encoded and __typeof(source_value) is unportable. But we already had to use knowledge of its type to hard-code the decoding method as le32toh(). So we may as well hard-code the type too: uint32_t npart; /* XXX: assume that dlp_npartitions is u32 */ ... npart = le32toh(dlp->d_npartitions); if (npart < 1 || npart > MAXPARTITIONS64) goto invalid_label; basetable->gpt_entries = npart; Old struct disklabel has the same design bug, but its d_npartitions has type uint16_t. With 32-bit ints, uint16_t promotes to plain int, so its unsigned features are hard to use even if you want them. Expressions like le16toh(dlp->d_npartitions) - 1 worked as intended but rarely as wanted after C90 standardised the bad "value-preserving" sign extension rules: when the source value is 0, the value of this subtraction is -1 with a 16-bit source but 0xFFFFFFFFU with a 32-bit source (assuming a 32-bit int target -- the general case is more complicated). This makes loops difficult to write. If you use an index variable i then it should have type int, but might need to be uint32_t depending on the source type and compiler warnings. If you use the entry in the struct more directly, then counting down from -1 to 0 just won't work if the type in the struct is unsigned, except in some contexts where this type is smaller than int and the "value-preserving" rule gives the normally-unwanted value of -1. The decoding step prevents direct use in most cases. Thus it is very natural to fix unsigned design errors in standard structs -- you have to check bounds when converting, and the bounds usually don't go above INT16_MAX, so you can use signed types in your own structs. gpt->gpt_entries and the index variable ('index') for the loop that counts it down actually have the correct type (int), so point (1) is incorrect -- a negative or zero initial index just results in signed 0 being correctly counted down to -1 so that the loop iterates 0 times. Also, even with unsigned types, there would be no undefined behaviour -- 0U would be counted down to UINT_MAX, and assigning this to 'int index' would only cause implementation-defined behaviour. Nothing bad seems to happen for zero entries when the loop does nothing. Zero entries might even be a valid format; however, all disklabels are supposed to have a 'c' partition which takes 3 entries to reach, so 1 or 2 entries are just as invalid as ones with 0 entries. Bruce