Date: Sun, 27 Jul 2008 20:40:06 +0200 From: Roman Divacky <rdivacky@freebsd.org> To: Chagin Dmitry <chagin.dmitry@gmail.com> Cc: Alexander Leidinger <Alexander@leidinger.net>, freebsd-emulation@freebsd.org Subject: Re: kern/117010: [linux] linux_getdents() get somethinng like buffer overflow Message-ID: <20080727184006.GA99255@freebsd.org> In-Reply-To: <alpine.BSF.1.10.0807271958020.3912@ora.chd.net> References: <200807250700.m6P70FSF036132@freefall.freebsd.org> <20080726091045.4c617dc7@deskjail> <alpine.BSF.1.10.0807271958020.3912@ora.chd.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Jul 27, 2008 at 09:05:03PM +0400, Chagin Dmitry wrote:
> On Sat, 26 Jul 2008, Alexander Leidinger wrote:
>
> >Quoting Chagin Dmitry <chagin.dmitry@gmail.com> (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 <chagin.dmitry@gmail.com>
> >>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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080727184006.GA99255>