kern/117010: [linux] linux_getdents() get somethinng like buffer overflow

[Chagin Dmitry]

On Mon, 28 Jul 2008, Alexander Leidinger wrote:

> Quoting "Chagin Dmitry" <chagin.dmitry at 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 at 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 at gmail.com>
>>>> To: bug-followup at freebsd.org, samflanker at 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