From owner-freebsd-emulation@FreeBSD.ORG Mon Jul 28 10:23:22 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 CF1451065674 for ; Mon, 28 Jul 2008 10:23:22 +0000 (UTC) (envelope-from chagin.dmitry@gmail.com) Received: from fk-out-0910.google.com (fk-out-0910.google.com [209.85.128.187]) by mx1.freebsd.org (Postfix) with ESMTP id 26B0E8FC16 for ; Mon, 28 Jul 2008 10:23:21 +0000 (UTC) (envelope-from chagin.dmitry@gmail.com) Received: by fk-out-0910.google.com with SMTP id k31so4415981fkk.11 for ; Mon, 28 Jul 2008 03:23:20 -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=WrnaihLZ3P9g5dVWswQJATOVtZimkKq6w5EXxS7LLGY=; b=nDNb6wkNaAszS9thM/yqNY0w5dkI9QIeFfeGvorzVglDJQP0r+yKOTBC9Xqcx0PVhU 9KjKD12GeabIHgdE1I7RpKuH6x8apKzTo8XBL0KRaTq1M60gXNhhBvYi5B1K2bvsgVhV t5aCKw12O3UlDGLTdwOgop+pUoC+Yg2aeYLok= 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=BewoPtUqwe3VaR1hMFznqlSY0y8IunOYAvhYRtWoninIDSsPq9jkvMQCHSCukHIqe6 LHSQ1EG3sMqkISrPZH76UfeXolIpskMUDLVkWxN/CJOUK8g9FwVCjJdMa2u35M7h5LZK V7u1hBiaXi7ixpxkxXKRgS4djhE7HbxkbT4k4= Received: by 10.180.231.20 with SMTP id d20mr1327204bkh.11.1217240600102; Mon, 28 Jul 2008 03:23:20 -0700 (PDT) Received: from ora.chd.net ( [78.107.232.239]) by mx.google.com with ESMTPS id 28sm3056426fkx.1.2008.07.28.03.23.17 (version=TLSv1/SSLv3 cipher=RC4-MD5); Mon, 28 Jul 2008 03:23:19 -0700 (PDT) Date: Mon, 28 Jul 2008 14:23:03 +0400 (MSD) To: Alexander Leidinger In-Reply-To: <20080728085403.58063b2gbchdjtic@webmail.leidinger.net> Message-ID: References: <200807250700.m6P70FSF036132@freefall.freebsd.org> <20080726091045.4c617dc7@deskjail> <20080728085403.58063b2gbchdjtic@webmail.leidinger.net> User-Agent: Alpine 1.10 (BSF 962 2008-03-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; format=flowed; charset=US-ASCII 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: Mon, 28 Jul 2008 10:23:22 -0000 On Mon, 28 Jul 2008, Alexander Leidinger wrote: > Quoting "Chagin Dmitry" (from Sun, 27 Jul 2008 > 21:05:03 +0400 (MSD)): > >> 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. > > When you look at the FreeBSD manpage of dirent, it's not that surprising: > ---snip--- > /* > * The dirent structure defines the format of directory entries returned > by > * the getdirentries(2) system call. > * > * A directory entry has a struct dirent at the front of it, 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 > terminated. > * The maximum length of a name in a directory is MAXNAMLEN. > */ > ---snip--- > ups... tnhx!, but what for we do here this check? IMO, it is excessive. >>>> 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. > > len is the memory region where you construct the linux response, isn't it? > you are mistaken here, len points to FreeBSD buffer filled by vop_readdir >>> 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. > > Seems there was a part missing above... "lenght in bytes" = remaining length > in bytes. The important part is the use of the macro. The linux reclen macro > calculates some linux stuff + some freebsd stuff without any limit checks. > What happens if the size of the name member of the struct changes in > FreeBSD!?! Even if they _may_ be the same currently, this is dangerous. > agree, we should do check before calculating linuxreclen, like: if (bdp->d_namlen > LINUX_NAME_MAX) { error = ENAMETOOLONG; goto out; } >>> 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 > > When I look at this, I even see more dragons in our code. In linux (2.6 > kernel) linux_dirent is playing the ARRAY[1] + size trick, in FreeBSD it > isn't. Some things are handled like in linux, but because the trick is not > done in FreeBSD, those can not be handled like in linux. > > When I look at the patch you proposed, I also see a pitfall. In linux in the > 64bit case, it's "int reclen = ALIGN(NAME_OFFSET(dirent) + namlen + 1, > sizeof(u64));", in the 32bit case it's "int reclen = > ALIGN(NAME_OFFSET(dirent) + namlen + 2, sizeof(long));". This means the > length is aligned to 64bit for the 64bit case and 32bit in the 32bit case. > ah.., getdents64 on all $arch's uses 64bit alignment, we should follow this behaviour. >>>> 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' > > There should be a check before data is copied to the place. As the size is > already available, it doesn't cost much. > > I try to get some time this week to produce a patch which addresses my > concerns. > ok, I shall test on amd64 :) thnx! -- Have fun! chd