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

[Alexander Leidinger]

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---

>>>  		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?

>> 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.

>> 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.

>>> 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.

Bye,
Alexander.

-- 
Moebius always does it on the same side.

http://www.Leidinger.net    Alexander @ Leidinger.net: PGP ID = B0063FE7
http://www.FreeBSD.org       netchild @ FreeBSD.org  : PGP ID = 72077137