Re: [issue1863] Implement 'hammer volume-list' subcommand

[Aggelos Economopoulos <aoiko@xxxxxxxxxxxxxx>]

On 10/07/2010 10:17 PM, Aggelos Economopoulos wrote:
> On 10/07/2010 08:02 PM, Stathis Kamperis (via DragonFly issue tracker)
> wrote:
>>
>> Stathis Kamperis <ekamperi@gmail.com> added the comment:
>>
>> Short follow-up.
>>
>> Matt commented on the code in IRC and said that there should a validation of
>> sizeof(struct hammer_ioc_volume). Otherwise the hammer vfs might overflow the
>> data buffer, the userland provides.
>>
>> Although Matt was kind enough to explain it twice, I still don't get it. I'm
>> allocating room for the maximum volumes a file system can have and also I'm only
>> writing to the 'device_name' field of 'hammer_ioc_volume' structure, which
>> happens to have automatic storage.
> 
> I haven't read the irc discussion, but skimming through your patch I see
> the following issues:
> 
> * In hammer_ioc_volume_list() you seem to be directly dereferencing the
> userland pointer. You should be using copyin() and friends to bring in
> the actual pointer and the count.


Ah, ioctl automagically copies in the struct. So no problem there.

> * I can see you are correctly specifying the max volume number in
> hammer(8), but the issue is that the kernel side should be making sure
> not to overrun the buffer space. Your kernel loop copies
> HAMMER_MAX_VOLUMES irrespective of how much space (possibly zero) the
> user side has specified. Your hammer(8) happens to behave correctly, but
> anyone can compile and run a program that will panic the kernel.
> 
> * Again, you cannot just strlcpy() stuff to userspace. The page might
> not be present or the address could be completely invalid. Use copyout
> instead.
>
> * I see no need to define an extra structure. Just pass the two fields
> in as arguments to the ioctl().

Silly me, of course you need to use the structure. The other two issues
are valid though.

Aggelos