From owner-freebsd-emulation@FreeBSD.ORG Mon Jul 28 06:54:15 2008 Return-Path: Delivered-To: freebsd-emulation@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8EBD31065679 for ; Mon, 28 Jul 2008 06:54:15 +0000 (UTC) (envelope-from alexander@leidinger.net) Received: from redbull.bpaserver.net (redbullneu.bpaserver.net [213.198.78.217]) by mx1.freebsd.org (Postfix) with ESMTP id ECFB38FC1E for ; Mon, 28 Jul 2008 06:54:14 +0000 (UTC) (envelope-from alexander@leidinger.net) Received: from outgoing.leidinger.net (p54A547B7.dip.t-dialin.net [84.165.71.183]) by redbull.bpaserver.net (Postfix) with ESMTP id C097F2E0A5; Mon, 28 Jul 2008 08:54:07 +0200 (CEST) Received: from webmail.leidinger.net (webmail.leidinger.net [192.168.1.102]) by outgoing.leidinger.net (Postfix) with ESMTP id 041F968B3D; Mon, 28 Jul 2008 08:54:04 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=Leidinger.net; s=outgoing-alex; t=1217228044; bh=zqq/p7YLel3bfUPC0omhQ0CF4830F0kIg BH+lMe69ow=; h=Message-ID:Date:From:To:Cc:Subject:References: In-Reply-To:MIME-Version:Content-Type:Content-Transfer-Encoding; b=n6BP+z6BwFlrMy7sajdUQy56iaqH+vk9sntG8/fgi5Eap43AWlSQqnnSHl3drGFA7 lztiLp7grihvemvhFry5RRxJk4iMny2L6VF3lL0iq1U/a/wZUyzCa/DzLSLmgVwiT/M 6fWxpcf+CxfNebjobbQw/nrdtdshl53Igi8xf/yo5kBcYJrNCL3EVd+Rk1JXtUrFAeK 4Uc11N+KawFwYWYx0B9JooScrL0EaQ4bbb8Fm5g+dnNKPxQsZtmub1ySGhBhdk3qp7W d0dsBlxfEu3PPH3xDEkoqVBqY6GnBoAjDCrKJvyDs8W21TYrK+ow9PnUuvNT5Rdz0NN UwgL5/lig== Received: (from www@localhost) by webmail.leidinger.net (8.14.2/8.13.8/Submit) id m6S6s3R3055386; Mon, 28 Jul 2008 08:54:03 +0200 (CEST) (envelope-from Alexander@Leidinger.net) Received: from pslux.cec.eu.int (pslux.cec.eu.int [158.169.9.14]) by webmail.leidinger.net (Horde Framework) with HTTP; Mon, 28 Jul 2008 08:54:03 +0200 Message-ID: <20080728085403.58063b2gbchdjtic@webmail.leidinger.net> X-Priority: 3 (Normal) Date: Mon, 28 Jul 2008 08:54:03 +0200 From: "Alexander Leidinger" To: "Chagin Dmitry" References: <200807250700.m6P70FSF036132@freefall.freebsd.org> <20080726091045.4c617dc7@deskjail> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; DelSp="Yes"; format="flowed" Content-Disposition: inline Content-Transfer-Encoding: quoted-printable User-Agent: Internet Messaging Program (IMP) H3 (4.2) / FreeBSD-8.0 X-BPAnet-MailScanner-Information: Please contact the ISP for more information X-MailScanner-ID: C097F2E0A5.0D198 X-BPAnet-MailScanner: Found to be clean X-BPAnet-MailScanner-SpamCheck: not spam, ORDB-RBL, SpamAssassin (not cached, score=-13.927, required 6, BAYES_00 -15.00, DKIM_SIGNED 0.00, DKIM_VERIFIED -0.00, MIME_QP_LONG_LINE 1.40, RDNS_DYNAMIC 0.10, SMILEY -0.50, TW_BD 0.08) X-BPAnet-MailScanner-From: alexander@leidinger.net X-Spam-Status: No Cc: freebsd-emulation@freebsd.org Subject: Re: kern/117010: [linux] linux_getdents() get somethinng like buffer overflow X-BeenThere: freebsd-emulation@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Development of Emulators of other operating systems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Jul 2008 06:54:15 -0000 Quoting "Chagin Dmitry" (from Sun, 27 Jul =20 2008 21:05:03 +0400 (MSD)): > On Sat, 26 Jul 2008, Alexander Leidinger wrote: > >> Quoting Chagin Dmitry (Fri, 25 Jul 2008 =20 >> 07:00:15 GMT): >> >>> The following reply was made to PR kern/117010; it has been noted by GNA= TS. >>> >>> From: Chagin Dmitry >>> To: bug-followup@freebsd.org, samflanker@gmail.com >>> Cc: >>> Subject: Re: kern/117010: [linux] linux_getdents() get somethinng =20 >>> like buffer >>> overflow >>> Date: Fri, 25 Jul 2008 10:22:46 +0400 (MSD) >>> >>> Please, try a patch below: >>> >>> diff --git a/src/sys/compat/linux/linux_file.c =20 >>> b/src/sys/compat/linux/linux_file >>> index 303bc3f..d88f95f 100644 >>> --- a/src/sys/compat/linux/linux_file.c >>> +++ b/src/sys/compat/linux/linux_file.c >>> @@ -303,8 +303,8 @@ struct l_dirent64 { >>> =09char d_name[LINUX_NAME_MAX + 1]; >>> }; >>> >>> -#define LINUX_RECLEN(de,namlen) \ >>> - ALIGN((((char *)&(de)->d_name - (char *)de) + (namlen) + 1)) >>> +#define LINUX_RECLEN(de,namlen,trail) \ >>> + ALIGN((((char *)&(de)->d_name - (char *)de) + (namlen) + trail)) >> >> >> The start of de->d_name minus the start of de should be the same as =20 >> the offset of d_name in de, so I would expect that this is =20 >> expressed with the offsetof maro instead of handmade. >> >> So the result of this is the offset plus a len + something. >> > > well... agree. > >>> #define LINUX_DIRBLKSIZ 512 >>> >>> @@ -436,8 +436,8 @@ again: >>> =09=09} >> >> I try to understand the code before this. There's "if (reclen & 3)" =20 >> error out. Does it mean it has to be a multiple of 4? If yes it =20 >> should be changed to some modulo calculation to make it obvious =20 >> (the compiler should be able to do such micro optimisations, but I =20 >> doubt the error case needs to be micro optimized). >> > > this code looks as a workaround... exists since v1.1, I don't =20 > understand what is it. When you look at the FreeBSD manpage of dirent, it's not that surprising: ---snip--- /* * The dirent structure defines the format of directory entries =20 returned by * the getdirentries(2) system call. * * A directory entry has a struct dirent at the front of it, =20 containing its * inode number, the length of the entry, and the length of the name * contained in the entry. These are followed by the name padded to a= 4 * byte boundary with null bytes. All names are guaranteed null =20 terminated. * The maximum length of a name in a directory is MAXNAMLEN. */ ---snip--- >>> =09=09linuxreclen =3D (is64bit) >>> - ? LINUX_RECLEN(&linux_dirent64, bdp->d_namlen) >>> - : LINUX_RECLEN(&linux_dirent, bdp->d_namlen); >>> + ? LINUX_RECLEN(&linux_dirent64, bdp->d_namlen, 1) >>> + : LINUX_RECLEN(&linux_dirent, bdp->d_namlen, 2); >> >> Translated: The length of the linux record is the offset plus the =20 >> FreeBSD size plus something. Doesn't make sense to me. =20 >> sizeof(linux_dirent) sound more suitable for this variable name. =20 >> From the code it can not be the length of the linux record, but the =20 >> size of a linux dirent struct which would be required to put all =20 >> info inside (+ some more space... very suspicious). >> >>> =09=09if (reclen > len || resid < linuxreclen) { >>> =09=09=09outp++; >> >> First part: if the length of the current record is larger than the =20 >> remaining free space (if it does not fit) go out of the loop... ok. >> > > no, here reclen is the length of FreeBSD record and len is the =20 > remaining space in FreeBSD records buffer. len is the memory region where you construct the linux response, isn't it? >> Second part: if the length (in bytes?) is smaller than the =20 >> theoretical size of the linux struct, go out of the loop. Ouch. =20 >> Please tell me this is wrong (I didn't had breakfast yet, I really =20 >> hope I misanalysed this because of this fact). >> > > no, resid is the free space in Linux records buffer, linuxreclen is =20 > the length of the Linux record. Seems there was a part missing above... "lenght in bytes" =3D remaining =20 length in bytes. The important part is the use of the macro. The linux =20 reclen macro calculates some linux stuff + some freebsd stuff without =20 any limit checks. What happens if the size of the name member of the =20 struct changes in FreeBSD!?! Even if they _may_ be the same currently, =20 this is dangerous. >> I smell buffer mismanagement because of the strange 1 or 2 being =20 >> added to the size, and I smell some convoluted logic there. Instead =20 >> of trying to poke the thing until it works, I suggest to step back =20 >> and have a look at the big picture if the entire part of the =20 >> function can be improved. > > This is the Linux, as Roman says - linux is a really strange :) > See linux source fs/readdir.c implementation of filldir functions When I look at this, I even see more dragons in our code. In linux =20 (2.6 kernel) linux_dirent is playing the ARRAY[1] + size trick, in =20 FreeBSD it isn't. Some things are handled like in linux, but because =20 the trick is not done in FreeBSD, those can not be handled like in =20 linux. When I look at the patch you proposed, I also see a pitfall. In linux =20 in the 64bit case, it's "int reclen =3D ALIGN(NAME_OFFSET(dirent) + =20 namlen + 1, sizeof(u64));", in the 32bit case it's "int reclen =3D =20 ALIGN(NAME_OFFSET(dirent) + namlen + 2, sizeof(long));". This means =20 the length is aligned to 64bit for the 64bit case and 32bit in the =20 32bit case. >>> it solves getdents() problem (at least at x86_64 emulation with >>> linux_base-f8) >>> >>> ps, be not bared, linux really has such features... >> >> What I would expect is to compare the strlen of the FreeBSD record =20 >> with the size of the place in linux_dirent. If the FreeBSD record =20 >> does not fit, fail (ENAMETOOLONG?). Compare the remaining space =20 >> with the size of linux_dirent, if it is '<=3D' fill in the data into =20 >> the fixed size struct. >> > > It's done in the 'Second part' There should be a check before data is copied to the place. As the =20 size is already available, it doesn't cost much. I try to get some time this week to produce a patch which addresses my =20 concerns. Bye, Alexander. --=20 Moebius always does it on the same side. http://www.Leidinger.net Alexander @ Leidinger.net: PGP ID =3D B0063FE7 http://www.FreeBSD.org netchild @ FreeBSD.org : PGP ID =3D 72077137