Date:	Tue, 17 May 2016 20:04:59 +0100
From:	Emil Velikov <emil.l.velikov@...il.com>
To:	Benjamin Gaignard <benjamin.gaignard@...aro.org>
Cc:	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	"Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>,
	ML dri-devel <dri-devel@...ts.freedesktop.org>,
	Zoltan Kuscsik <zoltan.kuscsik@...aro.org>,
	Sumit Semwal <sumit.semwal@...aro.org>,
	Cc Ma <cc.ma@...iatek.com>,
	Pascal Brand <pascal.brand@...aro.org>,
	Joakim Bech <joakim.bech@...aro.org>,
	Dan Caprita <dan.caprita@...driver.com>
Subject: Re: [PATCH v7 1/3] create SMAF module

On 17 May 2016 at 14:50, Benjamin Gaignard <benjamin.gaignard@...aro.org> wrote:
> Hello Emil,
>
> thanks for your review.
> I have understand most of your remarks and I'm fixing them
> but some points aren't obvious for me...
>
Sure thing. Thanks for being honest.

>
> No because a device could attach itself on the buffer and the
> allocator will only
> be selected at the first map_attach call.
> The goal is to delay the allocation until all devices are attached to
> select the best allocator.
>
I see. Makes sense.


>>> +static long smaf_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>>> +{
>>> +       switch (cmd) {
>>> +       case SMAF_IOC_CREATE:
>>> +       {
>>> +               struct smaf_create_data data;
>>> +               struct smaf_handle *handle;
>>> +
>>> +               if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
>>> +                       return -EFAULT;
>>> +
>>> +               handle = smaf_create_handle(data.length, data.flags);
>> We want to sanitise the input data.{length,flags} before sending it
>> deeper in the kernel.
>
> Sorry but can you elaborate little more here ?
> I don't understand your expectations.
>
You want to determine which flags are 'considered useful' at this
stage, and reject anything else. As-is you inject any flags that the
user gives you directly into the 'guts' of the kernel. This is not
good from security and future expandability POV.

About the length you want a similar thing. size_t is unsigned (great),
although ideally you'll want to check/determine if one cannot exploit
it, integer overflow being the more common suspect. This may be quite
hard to track, so I'd stick with the flags checking at least.


> It is useless the add this function in this .h file I will remove it
> and fix the comment in structure defintion

I've seen both approaches - description next to the declaration or
definition. I'd rather not pick sides, as people might throw rotten
fruit at me ;-)


>>> +/**
>>> + * struct smaf_create_data - allocation parameters
>>> + * @length:    size of the allocation
>>> + * @flags:     flags passed to allocator
>>> + * @name:      name of the allocator to be selected, could be NULL
Just occurred to - you might want to comment what the user should
expect if NULL. Any at random one will be used or otherwise. Very
quick description on the heuristics used might be good as well.

>> Is it guaranteed to be null terminated ? If so one should mentioned it
>> otherwise your userspace should be fixed.
>> Same comments apply for smaf_info::name.
>
> I have used strncpy everywhere to avoid this problem but maybe it is not enough
>
According to the man page

"The strncpy() function is similar, except that at most n bytes of src
are copied.  Warning: If there is no null byte among the first n bytes
of src, the string placed in dest will _not_ be null-terminated."

Annotation is mine obviously. I believe that after the strncpy 'name'
is/was assumed (used as) a NULL terminated string.

>>
>>
>>> + * @fd:                returned file descriptor
>>> + */
>>> +struct smaf_create_data {
>>> +       size_t length;
>>> +       unsigned int flags;
>>> +       char name[ALLOCATOR_NAME_LENGTH];
>>> +       int fd;
>> The structs here feels quite fragile. Please read up on Daniel
>> Vetter's "Botching up ioctls" [1]. Personally I find pahole quite
>> useful is such process.
>>
> if I describe the structures like this:
> /**
>  * struct smaf_create_data - allocation parameters
>  * @length: size of the allocation
>  * @flags: flags passed to allocator
>  * @name_len: length of name
>  * @name: name of the allocator to be selected, could be NULL
>  * @fd: returned file descriptor
>  */
> struct smaf_create_data {
> size_t length;
> unsigned int flags;
> size_t name_len;
> char __user *name;
> int fd;
> char padding[44];
> };
>
> does it sound more robust for you ?
>
Seems like you changed 'name' from fixed size array to char *. Which
actually gets us slightly further away from robust.

As Daniel said, please read through the hole file.

Here is a (slightly incomplete) gist of it all:
- you want to to use __[us]{8,16,32,64} and __kernel_size_t types everywhere
- each member of the struct must be the same offset for both 32bit and
64bit builds. ^^ helps with that
- double check for gaps in the struct - I think you have a few
- each struct should have it's old 'flags' which you'll use to
indicate the capability of the ioctl and thus the size of struct used.
i.e. it's for future use.

Obviously the other two structs need similar polish.

Here is how you can check things with pahole:
 - Create a simple C file that includes the header and has an instance
of each struct - it doesn't have to be use any of them.
 - Compile for 32 and 64 bit with -g -O0. Compare the struct layout -
it should be identical in both cases.

Hope that makes things a bit clearer.

Emil

Powered by blists - more mailing lists