Date: Mon, 28 Jul 2008 14:23:03 +0400 (MSD) From: Chagin Dmitry <chagin.dmitry@gmail.com> To: Alexander Leidinger <Alexander@leidinger.net> Cc: freebsd-emulation@freebsd.org, Chagin Dmitry <chagin.dmitry@gmail.com> Subject: Re: kern/117010: [linux] linux_getdents() get somethinng like buffer overflow Message-ID: <alpine.BSF.1.10.0807281300060.1453@ora.chd.net> In-Reply-To: <20080728085403.58063b2gbchdjtic@webmail.leidinger.net> References: <200807250700.m6P70FSF036132@freefall.freebsd.org> <20080726091045.4c617dc7@deskjail> <alpine.BSF.1.10.0807271958020.3912@ora.chd.net> <20080728085403.58063b2gbchdjtic@webmail.leidinger.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 28 Jul 2008, Alexander Leidinger wrote:
> Quoting "Chagin Dmitry" <chagin.dmitry@gmail.com> (from Sun, 27 Jul 2008
> 21:05:03 +0400 (MSD)):
>
>> 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.
>
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.BSF.1.10.0807281300060.1453>