Date: Sat, 26 Jul 2008 09:10:45 +0200 From: Alexander Leidinger <Alexander@Leidinger.net> To: Chagin Dmitry <chagin.dmitry@gmail.com> Cc: freebsd-emulation@FreeBSD.org Subject: Re: kern/117010: [linux] linux_getdents() get somethinng like buffer overflow Message-ID: <20080726091045.4c617dc7@deskjail> In-Reply-To: <200807250700.m6P70FSF036132@freefall.freebsd.org> References: <200807250700.m6P70FSF036132@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
> #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).
> 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.
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).
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.
> 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.
Bye,
Alexander.
--
The best way to inspire fresh thoughts is to seal
the letter.
http://www.Leidinger.net Alexander @ Leidinger.net: PGP ID = B0063FE7
http://www.FreeBSD.org netchild @ FreeBSD.org : PGP ID = 72077137
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080726091045.4c617dc7>