Date: Mon, 28 Jul 2008 08:54:03 +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: <20080728085403.58063b2gbchdjtic@webmail.leidinger.net> 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
Quoting "Chagin Dmitry" <chagin.dmitry@gmail.com> (from Sun, 27 Jul =20
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 =20
>> 07:00:15 GMT):
>>
>>> The following reply was made to PR kern/117010; it has been noted by GNA=
TS.
>>>
>>> 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 =20
>>> 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 =20
>>> 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 {
>>> =09char 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 =20
>> the offset of d_name in de, so I would expect that this is =20
>> 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:
>>> =09=09}
>>
>> I try to understand the code before this. There's "if (reclen & 3)" =20
>> error out. Does it mean it has to be a multiple of 4? If yes it =20
>> should be changed to some modulo calculation to make it obvious =20
>> (the compiler should be able to do such micro optimisations, but I =20
>> doubt the error case needs to be micro optimized).
>>
>
> this code looks as a workaround... exists since v1.1, I don't =20
> 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 =20
returned by
* the getdirentries(2) system call.
*
* A directory entry has a struct dirent at the front of it, =20
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 =20
terminated.
* The maximum length of a name in a directory is MAXNAMLEN.
*/
---snip---
>>> =09=09linuxreclen =3D (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 =20
>> FreeBSD size plus something. Doesn't make sense to me. =20
>> sizeof(linux_dirent) sound more suitable for this variable name. =20
>> From the code it can not be the length of the linux record, but the =20
>> size of a linux dirent struct which would be required to put all =20
>> info inside (+ some more space... very suspicious).
>>
>>> =09=09if (reclen > len || resid < linuxreclen) {
>>> =09=09=09outp++;
>>
>> First part: if the length of the current record is larger than the =20
>> 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 =20
> remaining space in FreeBSD records buffer.
len is the memory region where you construct the linux response, isn't it?
>> Second part: if the length (in bytes?) is smaller than the =20
>> theoretical size of the linux struct, go out of the loop. Ouch. =20
>> Please tell me this is wrong (I didn't had breakfast yet, I really =20
>> hope I misanalysed this because of this fact).
>>
>
> no, resid is the free space in Linux records buffer, linuxreclen is =20
> the length of the Linux record.
Seems there was a part missing above... "lenght in bytes" =3D remaining =20
length in bytes. The important part is the use of the macro. The linux =20
reclen macro calculates some linux stuff + some freebsd stuff without =20
any limit checks. What happens if the size of the name member of the =20
struct changes in FreeBSD!?! Even if they _may_ be the same currently, =20
this is dangerous.
>> I smell buffer mismanagement because of the strange 1 or 2 being =20
>> added to the size, and I smell some convoluted logic there. Instead =20
>> of trying to poke the thing until it works, I suggest to step back =20
>> and have a look at the big picture if the entire part of the =20
>> 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 =20
(2.6 kernel) linux_dirent is playing the ARRAY[1] + size trick, in =20
FreeBSD it isn't. Some things are handled like in linux, but because =20
the trick is not done in FreeBSD, those can not be handled like in =20
linux.
When I look at the patch you proposed, I also see a pitfall. In linux =20
in the 64bit case, it's "int reclen =3D ALIGN(NAME_OFFSET(dirent) + =20
namlen + 1, sizeof(u64));", in the 32bit case it's "int reclen =3D =20
ALIGN(NAME_OFFSET(dirent) + namlen + 2, sizeof(long));". This means =20
the length is aligned to 64bit for the 64bit case and 32bit in the =20
32bit case.
>>> 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 =20
>> with the size of the place in linux_dirent. If the FreeBSD record =20
>> does not fit, fail (ENAMETOOLONG?). Compare the remaining space =20
>> with the size of linux_dirent, if it is '<=3D' fill in the data into =20
>> 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 =20
size is already available, it doesn't cost much.
I try to get some time this week to produce a patch which addresses my =20
concerns.
Bye,
Alexander.
--=20
Moebius always does it on the same side.
http://www.Leidinger.net Alexander @ Leidinger.net: PGP ID =3D B0063FE7
http://www.FreeBSD.org netchild @ FreeBSD.org : PGP ID =3D 72077137
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080728085403.58063b2gbchdjtic>