From owner-freebsd-emulation@FreeBSD.ORG Sun Jul 27 18:41:34 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 CFECA1065675 for ; Sun, 27 Jul 2008 18:41:34 +0000 (UTC) (envelope-from rdivacky@vlk.vlakno.cz) Received: from vlakno.cz (vlk.vlakno.cz [62.168.28.247]) by mx1.freebsd.org (Postfix) with ESMTP id 7546C8FC12 for ; Sun, 27 Jul 2008 18:41:33 +0000 (UTC) (envelope-from rdivacky@vlk.vlakno.cz) Received: from localhost (localhost [127.0.0.1]) by vlakno.cz (Postfix) with ESMTP id 568B567BA68; Sun, 27 Jul 2008 20:40:19 +0200 (CEST) X-Virus-Scanned: amavisd-new at vlakno.cz Received: from vlakno.cz ([127.0.0.1]) by localhost (vlk.vlakno.cz [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 2MRp-iLEYKCu; Sun, 27 Jul 2008 20:40:07 +0200 (CEST) Received: from vlk.vlakno.cz (localhost [127.0.0.1]) by vlakno.cz (Postfix) with ESMTP id 691C867AA9D; Sun, 27 Jul 2008 20:40:07 +0200 (CEST) Received: (from rdivacky@localhost) by vlk.vlakno.cz (8.14.2/8.14.2/Submit) id m6RIe65g099404; Sun, 27 Jul 2008 20:40:06 +0200 (CEST) (envelope-from rdivacky) Date: Sun, 27 Jul 2008 20:40:06 +0200 From: Roman Divacky To: Chagin Dmitry Message-ID: <20080727184006.GA99255@freebsd.org> References: <200807250700.m6P70FSF036132@freefall.freebsd.org> <20080726091045.4c617dc7@deskjail> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i Cc: Alexander Leidinger , 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: Sun, 27 Jul 2008 18:41:34 -0000 On Sun, Jul 27, 2008 at 09:05:03PM +0400, Chagin Dmitry wrote: > On Sat, 26 Jul 2008, Alexander Leidinger wrote: > > >Quoting Chagin Dmitry (Fri, 25 Jul 2008 07:00:15 > >GMT): > > > >>The following reply was made to PR kern/117010; it has been noted by > >>GNATS. > >> > >>From: Chagin Dmitry > >>To: bug-followup@freebsd.org, samflanker@gmail.com > >>Cc: > >>Subject: Re: kern/117010: [linux] linux_getdents() get somethinng 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 > >> 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 { > >> char 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 the > >offset of d_name in de, so I would expect that this is 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: > >> } > > > >I try to understand the code before this. There's "if (reclen & 3)" error > >out. Does it mean it has to be a multiple of 4? If yes it should be > >changed to some modulo calculation to make it obvious (the compiler should > >be able to do such micro optimisations, but I doubt the error case needs > >to be micro optimized). > > > > this code looks as a workaround... exists since v1.1, I don't understand > what is it. > > > >> linuxreclen = (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 FreeBSD > >size plus something. Doesn't make sense to me. sizeof(linux_dirent) sound > >more suitable for this variable name. From the code it can not be the > >length of the linux record, but the size of a linux dirent struct which > >would be required to put all info inside (+ some more space... very > >suspicious). > > > >> if (reclen > len || resid < linuxreclen) { > >> outp++; > > > >First part: if the length of the current record is larger than the > >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 remaining > space in FreeBSD records buffer. > > > >Second part: if the length (in bytes?) is smaller than the theoretical > >size of the linux struct, go out of the loop. Ouch. Please tell me this is > >wrong (I didn't had breakfast yet, I really hope I misanalysed this > >because of this fact). > > > > no, resid is the free space in Linux records buffer, linuxreclen is the > length of the Linux record. > > >I smell buffer mismanagement because of the strange 1 or 2 being added to > >the size, and I smell some convoluted logic there. Instead of trying to > >poke the thing until it works, I suggest to step back and have a look at > >the big picture if the entire part of the 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 I'll look at the readdir.c implementation, analyze it but I guess Dmitry's version is ok. if I find it correct I think we should wait for someone to test it but if noone does I think it can be commited with just Dmitry's testing and my analysis :) roman