From owner-svn-src-all@FreeBSD.ORG Mon Jul 18 09:11:58 2011 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B0E59106564A; Mon, 18 Jul 2011 09:11:58 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail05.syd.optusnet.com.au (mail05.syd.optusnet.com.au [211.29.132.186]) by mx1.freebsd.org (Postfix) with ESMTP id 345228FC12; Mon, 18 Jul 2011 09:11:57 +0000 (UTC) Received: from c122-106-165-191.carlnfd1.nsw.optusnet.com.au (c122-106-165-191.carlnfd1.nsw.optusnet.com.au [122.106.165.191]) by mail05.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id p6I9Br2q018110 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 18 Jul 2011 19:11:54 +1000 Date: Mon, 18 Jul 2011 19:11:53 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Ryan Stone In-Reply-To: Message-ID: <20110718180637.O1038@besplex.bde.org> References: <201107172108.p6HL8Gkd090278@svn.freebsd.org> <20110718080428.N3939@besplex.bde.org> MIME-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="0-1224280693-1310980313=:1038" Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans , markjdb@gmail.com Subject: Re: svn commit: r224150 - head/sbin/fdisk X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Jul 2011 09:11:58 -0000 This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --0-1224280693-1310980313=:1038 Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE On Sun, 17 Jul 2011, Ryan Stone wrote: > On Sun, Jul 17, 2011 at 7:09 PM, Bruce Evans wrote= : >> There was no need to further break the style. > > Ack. Should have caught that. Will fix. Thanks. >>> @@ -990,7 +990,7 @@ parse_config_line(char *line, CMD *comma >>> =A0 =A0 =A0 =A0 =A0 =A0if (isalpha(*cp)) >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0command->args[command->n_args].argtype = =3D *cp++; >>> =A0 =A0 =A0 =A0 =A0 =A0end =3D NULL; >>> - =A0 =A0 =A0 =A0 =A0 command->args[command->n_args].arg_val =3D strtol= (cp, &end, 0); >>> + =A0 =A0 =A0 =A0 =A0 command->args[command->n_args].arg_val =3D strtou= l(cp, &end, 0); >>> =A0 =A0 =A0 =A0 =A0 =A0if (cp =3D=3D end || (!isspace(*end) && *end != =3D '\0')) { >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0char ch; >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0end =3D cp; Arrgh, hard \xa0 what should be very plain text for a diff unreadable. >> Also, arg_val is never used as a u_long. =A0We risk truncation when it i= s >> blindly assigned to variables whose type is never u_long. =A0It is mostl= y >> assigned to variables of type uint ("uint" is an archaic SysV spelling >> of the BSD u_int which is a spelling of unsigned int. =A0This style bug >> was imported from Mach and never fixed). =A0Since uint is unsigned and i= nts >> have 32 bits, things now work up to UINT32_MAX but no further. =A0arg_va= l >> is also blindly assigned to a variable of type int, the partition number= , >> and to a variable of type u_char (the partition type), but since these >> values nonsense unless they are small, there is no problem with overflow >> near INT32_MAX or UINT32_MAX. > > Hm, I was under the impression that fdisk was checking whether the > size parameter fit within in 32-bits before putting it in the MBR, but > I seem to have been mistaken. In that case, I think that the right > fix is to make arg_val a u_int for now, which will still allow slices > with more than 2^31 sectors to be specified. Truncation is still an > issue but that's not a new bug. That would be good. The only other gross overflow bugs in fdisk that I know of are that when it gets parameters from the kernel, it blindly multiplies them and can overflow. get_params() also returns a nonsense status which is especially broken at 4G-1 and above. % ... % static int cyls, sectors, heads, cylsecs, disksecs; disksecs can overflow in practice. % ... % static uint dos_cyls; % static uint dos_heads; % static uint dos_sectors; % static uint dos_cylsecs; Better, and correct modulo checks that the assumptions that ints are 32 bits and that assignments to these variables check that the value is <=3D UINT32_MAX. % ... % static int % get_params() % { % =09int error; % =09u_int u; % =09off_t o; %=20 % =09error =3D ioctl(fd, DIOCGFWSECTORS, &u); % =09if (error =3D=3D 0) % =09=09sectors =3D dos_sectors =3D u; % =09else % =09=09sectors =3D dos_sectors =3D 63; %=20 % =09error =3D ioctl(fd, DIOCGFWHEADS, &u); % =09if (error =3D=3D 0) % =09=09heads =3D dos_heads =3D u; % =09else % =09=09heads =3D dos_heads =3D 255; Small values, so no problems in practice. Bad variable names, including using u as a temporary copy for variables with different semantics. Perfectly backwards sorting of the declarations. %=20 % =09dos_cylsecs =3D cylsecs =3D heads * sectors; Another small value in practice. % =09disksecs =3D cyls * heads * sectors; Can easily overflow. The last multiplication overflows at 4G sectors in practice. The assigment overflows at 2G sectors in practice. %=20 % =09u =3D g_sectorsize(fd); % =09if (u <=3D 0) % =09=09return (-1); %=20 % =09o =3D g_mediasize(fd); % =09if (o < 0) % =09=09return (-1); % =09disksecs =3D o / u; Can easily overflow at 2G sectors on assignment. % =09cyls =3D dos_cyls =3D o / (u * dos_heads * dos_sectors); No problems in practice. %=20 % =09return (disksecs); Nonsense return value. This function returns -1 on error, else it should return 0. It is only called once, and its caller actually checks the return value, and exits (after printing reasonable warning messages in open_disk(), then a bogus warning message and a bogus errno using err()) iff this function returns -1. So disksecs is a garbage value to return here. When the number of sectors is > INT_MAX, the garbage has already overflowed on assignment to disksecs. This is mostly benign up to UINT32_MAX (and even later -- see below), but here it is fatal at precisely UINT32_MAX, since that has overflowed to the supposed-to-be-out-of-band value -1 on assignment to disksecs (if it didn't overflow there, it would overflow on return). And this value is the most likely one for a large disk -- you can probably usefully use a large disk by using only the first UINT32_MAX sectors on it. Then either you or the kernel or the BIOS or error handling in strtoul() may have faked or otherwise produced the magic value of UINT32_MAX for the number of sectors. Or you can certainly usefully use this fdisk on a disk with more than UINT32_MAX sectors, provided you only want to change partitions lying entirely within the first UINT32_MAX sectors. % } Another bug in get_params() is that you have no direct control over the number of sectors. In the usual case, this number is the result of g_mediasize(). We depend on the broken error handling, and on this number not being -1 when truncated, to get past initialization to the point where we can specify the number of sectors interactively (or in the config file (?). Failing when the total number of sectors is unrepresentable in variables with wrong types would be especially silly, since fdisk only uses this value to initialize defaults (note that it is not part of CHS, and interactive mode doesn't support entering it. Interactive mode doesn't even fake it by multiplying out CHS -- it just multiplies HS -- see get_params_to_use(). Thus interactive mode doesn't have any of the overflow bugs in the above, and the overflow bugs in the above are more benign than first appears, since disksecs is rarely used and later uses of it, if any, are bugs since interactive mode doesn't maintain it). This also shows that we shouldn't be very strict in checking that values don't exceed UINT32_MAX. The values should only be checked if they are actually used. This is mainly partition offsets that will be written to disks. Partition sizes must not exceed UINT32_MAX, so checking that up front is harmless, but it is insufficient since the (offset + size) should not exceed UINT32_MAX. Hmm, I think the (offset, size) structure can support up to 8G-2 sectors if programmed carefully -- it has nothing to prevent starting a partition of size 4G-1 at offset 4G-1. So i386 MBRs should break at ~4TB, not at ~2TB with 512 byte sectors :-) (8 times that with 4K-sectors). Uncareful code like fdisk and probably BIOSes that at best just adds offset+size in uint32_t will of course break at 4G-1 sectors. Bruce --0-1224280693-1310980313=:1038--