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

[Aggelos Economopoulos <aoiko@xxxxxxxxxxxxxx>]

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.

* 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().

Nice job otherwise.

HTH,
Aggelos