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 <[email protected]> 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
