Bug #1863

Implement 'hammer volume-list' subcommand

Added by Anonymous about 4 years ago. Updated about 4 years ago.

Status:ClosedStart date:
Priority:LowDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

Description

First attempt to add a 'hammer volume-list' subcommand, that takes as argument a
filesystem and returns a list of volumes (device nodes) that make up that
filesystem.

http://stathisk.ath.cx/0001-HAMMER-Implement-volume-list-command.patch

Cheers,
Stathis

P.S. If anyone is willing to -instead- make mount(8) output the volumes for a
filesystem, it's fine by me.

History

#1 Updated by Anonymous about 4 years ago

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.

So, what kind of buffer overrun I should be checking against? Can anyone please
provide some code snippet or an insight ?

Thanks!
Stathis

#2 Updated by aoiko about 4 years ago

On 10/07/2010 08:02 PM, Stathis Kamperis (via DragonFly issue tracker)
wrote:
>
> Stathis Kamperis <> 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

#3 Updated by aoiko about 4 years ago

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 <> 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 Updated by Anonymous about 4 years ago

Thanks Aggelos. I'll fix the issues you raised.

Cheers,
Stathis

#5 Updated by Anonymous about 4 years ago

I updated the patch, same url:

http://stathisk.ath.cx/0001-HAMMER-Implement-volume-list-command.patch

Regards,
Stathis

#6 Updated by josepht about 4 years ago

On Fri, Oct 08, 2010 at 04:01:22PM +0000, Stathis Kamperis (via DragonFly issue tracker) wrote:
>
> Stathis Kamperis <> added the comment:
>
> I updated the patch, same url:
>
> http://stathisk.ath.cx/0001-HAMMER-Implement-volume-list-command.patch

Link seems broken.

Joe

#7 Updated by Anonymous about 4 years ago

Fixed, sorry.

I changed strlcpy() to copyout() and also added some rudimentary validation on
the count of volumes to write.

I honestly don't know how to make kernel defend against a lying user-land.

Regards,
Stathis

#8 Updated by aoiko about 4 years ago

On 10/08/2010 06:26 PM, Stathis Kamperis (via DragonFly issue tracker)
wrote:
>
> Stathis Kamperis <> added the comment:
>
> Fixed, sorry.
>
> I changed strlcpy() to copyout() and also added some rudimentary validation on
> the count of volumes to write.
>
> I honestly don't know how to make kernel defend against a lying user-land.

The kernel holds all the cards, just check absolutely everything
userland tells you against your own data structures :)

Patch looks ok, except that you don't want to unconditionally copy
MAXPATHLEN bytes to userland. The issue here isn't performance, but
information disclosure. Your code will copy to userspace whatever
happens to be allocated adjacent to the volume name in RAM. Perhaps an
implementation of copyoutstr() is in order?

HTH,
Aggelos

#9 Updated by dillon about 4 years ago

ioctls copy data into/out of kernel space for you, but the
size of the structure is built into the ioctl command so it
cannot change without changing the ioctl command. It's also
fairly limited. So anything that is potentially large needs
to be passed in as a separate user pointer embedded in
the ioctl command structure.

-Matt
Matthew Dillon
<>

#10 Updated by Anonymous about 4 years ago

#11 Updated by aoiko about 4 years ago

On 10/19/2010 10:59 PM, Stathis Kamperis (via DragonFly issue tracker)
wrote:
>
> Stathis Kamperis <> added the comment:
>
> New patch, same url:
> http://stathisk.ath.cx/0001-HAMMER-Implement-volume-list-command.patch

Can you explain why you use the 0001- prefix if you're going to use the
same url over and over again? It's just extra pain for us to rename your
patches, which is something I did, so now I can do the following, in
order to only review your changes relative to the last patch. Comments
inline.

$ diff -u 000{1,2}-HAMMER-Implement-volume-list-command.patch
-+ int i, cnt;
++ int i, cnt, len;
+
+ for (i = 0, cnt = 0; i < HAMMER_MAX_VOLUMES && cnt < ioc->nvols;
i++) {
+ volume = hammer_get_volume(hmp, i, &error);
@@ -213,8 +213,13 @@
+ }
+ KKASSERT(volume != NULL && error == 0);
+
++ len = strlen(volume->vol_name) + 1;
++ if (len > MAXPATHLEN) {
++ len = MAXPATHLEN;
++ volume->vol_name[len - 1] = '\0';
++ }
+ error = copyout(volume->vol_name,
ioc->vols[cnt].device_name,
-+ MAXPATHLEN);
++ len);

Looks good, although you should probably just KKASSERT(len <
MAXPATHLEN). Please commit.

Aggelos

#12 Updated by swildner about 4 years ago

On 10/19/2010 23:22, Aggelos Economopoulos wrote:
> On 10/19/2010 10:59 PM, Stathis Kamperis (via DragonFly issue tracker)
> wrote:
>>
>> Stathis Kamperis<> added the comment:
>>
>> New patch, same url:
>> http://stathisk.ath.cx/0001-HAMMER-Implement-volume-list-command.patch
>
> Can you explain why you use the 0001- prefix if you're going to use the
> same url over and over again? It's just extra pain for us to rename your
> patches, which is something I did, so now I can do the following, in
> order to only review your changes relative to the last patch.
> [...]

fetch -o ...

Sascha

#13 Updated by aoiko about 4 years ago

On 10/20/2010 02:00 AM, Sascha Wildner wrote:
> On 10/19/2010 23:22, Aggelos Economopoulos wrote:
>> On 10/19/2010 10:59 PM, Stathis Kamperis (via DragonFly issue tracker)
>> wrote:
>>>
>>> Stathis Kamperis<> added the comment:
>>>
>>> New patch, same url:
>>> http://stathisk.ath.cx/0001-HAMMER-Implement-volume-list-command.patch
>>
>> Can you explain why you use the 0001- prefix if you're going to use the
>> same url over and over again? It's just extra pain for us to rename your
>> patches, which is something I did, so now I can do the following, in
>> order to only review your changes relative to the last patch.
>> [...]
>
> fetch -o ...

Yes, that is the renaming I was refering to. The fact that it is easy
to do does not address the problems I pointed out, namely that it is
pointless to have a serial counter prefix and not increment it.
Addditionally, rewriting the patch means that it is the reviewers who
need to bother with keeping history (which, as demonstrated, *is*
useful) and of course I can no longer refer unambiguously to version 1
of the patch (My local numbering after the renames is 0-2. What's yours?).

Aggelos

#14 Updated by Anonymous about 4 years ago

git format-patch -1 automatically added the 0001- prefix, since I re-based my
patch and made it appear on top of the commit history.

Sorry for the inconvenience.

Speaking of fetch(1), could we make it automatically append an increasing
counter if the file to be fetched already exists on disk ?

Stathis

#15 Updated by dillon about 4 years ago

:git format-patch -1 automatically added the 0001- prefix, since I re-based my
:patch and made it appear on top of the commit history.
:
:Sorry for the inconvenience.
:
:Speaking of fetch(1), could we make it automatically append an increasing
:counter if the file to be fetched already exists on disk ?
:
:Stathis

I'd rather not. Many programs rely on fetch to operate just like it
does now.

-Matt
Matthew Dillon
<>

Also available in: Atom PDF