From owner-freebsd-emulation@FreeBSD.ORG Sun Jul 27 17:05:37 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 B8DA4106567A for ; Sun, 27 Jul 2008 17:05:37 +0000 (UTC) (envelope-from chagin.dmitry@gmail.com) Received: from fk-out-0910.google.com (fk-out-0910.google.com [209.85.128.186]) by mx1.freebsd.org (Postfix) with ESMTP id 41AC58FC1F for ; Sun, 27 Jul 2008 17:05:37 +0000 (UTC) (envelope-from chagin.dmitry@gmail.com) Received: by fk-out-0910.google.com with SMTP id k31so3888336fkk.11 for ; Sun, 27 Jul 2008 10:05:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:date:to:cc:subject :in-reply-to:message-id:references:user-agent:mime-version :content-type:from; bh=pZtGCrFupf4QN/Q88ohJrh7cPzCommpXuUQBikNBN9w=; b=SmCP/cLyuhjqH1+1rsSBwBbtUalxxgf0B8iNdO7wkVPFi/8SuYTilBsHOKlL752WtF GUwRSJANj4r9kt9QMKYSgQmxRUKaaiAlkjQBVItBI1N7n4zaSTrYmvtQ44y+qYniep3N GAL3Ai3L/gIVP8bE4zHQ398pieA/I4nDV8f/s= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:to:cc:subject:in-reply-to:message-id:references:user-agent :mime-version:content-type:from; b=lt0+YqYuvEexfGZalkgnQ4iR4L+12Ed4822JvuDkShqBpHeQ+JTfKlN5Mt5CSOoAq9 ZG3BTvxAZSQoqz6oF8JBXUrNezmJMjOLLGkx8GM+jFMLRLNTSkFSlhRC1azLB0uexHGL 6cD8aQrPUpfGqAIEwxG9FcD4gy9dItn5vaKmg= Received: by 10.180.235.10 with SMTP id i10mr1016924bkh.56.1217178335441; Sun, 27 Jul 2008 10:05:35 -0700 (PDT) Received: from ora.chd.net ( [78.107.232.239]) by mx.google.com with ESMTPS id 35sm2507675fkt.12.2008.07.27.10.05.33 (version=TLSv1/SSLv3 cipher=RC4-MD5); Sun, 27 Jul 2008 10:05:34 -0700 (PDT) Date: Sun, 27 Jul 2008 21:05:03 +0400 (MSD) To: Alexander Leidinger In-Reply-To: <20080726091045.4c617dc7@deskjail> Message-ID: References: <200807250700.m6P70FSF036132@freefall.freebsd.org> <20080726091045.4c617dc7@deskjail> User-Agent: Alpine 1.10 (BSF 962 2008-03-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed From: Chagin Dmitry Cc: freebsd-emulation@freebsd.org, Chagin Dmitry 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 17:05:37 -0000 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 > >> 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 with the size of the place in linux_dirent. If the FreeBSD record does not fit, fail (ENAMETOOLONG?). Compare the remaining space with the size of linux_dirent, if it is '<=' fill in the data into the fixed size struct. > It's done in the 'Second part' thnx! -- Have fun! chd