Project

General

Profile

Actions

Bug #1863

closed

Implement 'hammer volume-list' subcommand

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

Status:
Closed
Priority:
Low
Assignee:
-
Category:
-
Target version:
-
Start date:
Due date:
% Done:

0%

Estimated time:

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.

Actions #1

Updated by Anonymous about 11 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

Actions #2

Updated by aoiko about 11 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

Actions #3

Updated by aoiko about 11 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

Actions #4

Updated by Anonymous about 11 years ago

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

Cheers,
Stathis

Actions #5

Updated by Anonymous about 11 years ago

I updated the patch, same url:

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

Regards,
Stathis

Actions #6

Updated by josepht about 11 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

Actions #7

Updated by Anonymous about 11 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

Actions #8

Updated by aoiko about 11 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

Actions #9

Updated by dillon about 11 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
&lt;&gt;
Actions #10

Updated by Anonymous about 11 years ago

Actions #11

Updated by aoiko about 11 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;

+ 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

Actions #12

Updated by swildner about 11 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

Actions #13

Updated by aoiko about 11 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

Actions #14

Updated by Anonymous about 11 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

Actions #15

Updated by dillon about 11 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
&lt;&gt;
Actions

Also available in: Atom PDF