[issue1863] Implement 'hammer volume-list' subcommand
aoiko at cc.ece.ntua.gr
Thu Oct 7 14:40:11 PDT 2010
On 10/07/2010 10:17 PM, Aggelos Economopoulos wrote:
> On 10/07/2010 08:02 PM, Stathis Kamperis (via DragonFly issue tracker)
>> Stathis Kamperis <ekamperi at 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
> * 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.
More information about the Bugs