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

[Chagin Dmitry]

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.


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


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

> 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

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

thnx!

-- 
Have fun!
chd