Date: Mon, 18 Jul 2011 19:11:53 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ryan Stone <rysto32@gmail.com> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans <brde@optusnet.com.au>, markjdb@gmail.com Subject: Re: svn commit: r224150 - head/sbin/fdisk Message-ID: <20110718180637.O1038@besplex.bde.org> In-Reply-To: <CAFMmRNxcvLwh=pp5Wu_qmHWdQ78xUMmJtkpYymgxv6kHsFBgxQ@mail.gmail.com> References: <201107172108.p6HL8Gkd090278@svn.freebsd.org> <20110718080428.N3939@besplex.bde.org> <CAFMmRNxcvLwh=pp5Wu_qmHWdQ78xUMmJtkpYymgxv6kHsFBgxQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <brde@optusnet.com.au> 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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110718180637.O1038>