Re: [issue1863] Implement 'hammer volume-list' subcommand
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
討論串 (同標題文章)
完整討論串 (本文為第 4 之 17 篇)：