DragonFlyBSD

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

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

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 <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.

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

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

Cheers,
Stathis

I updated the patch, same url:

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

Regards,
Stathis

On Fri, Oct 08, 2010 at 04:01:22PM +0000, Stathis Kamperis (via DragonFly issue tracker) wrote:

Stathis Kamperis <ekamperi@gmail.com> added the comment:

I updated the patch, same url:

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

Link seems broken.

Joe

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

On 10/08/2010 06:26 PM, Stathis Kamperis (via DragonFly issue tracker)
wrote:

Stathis Kamperis <ekamperi@gmail.com> 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

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 
                    <dillon@backplane.com>

New patch, same url:
http://stathisk.ath.cx/0001-HAMMER-Implement-volume-list-command.patch

Stathis

On 10/19/2010 10:59 PM, Stathis Kamperis (via DragonFly issue tracker)
wrote:

Stathis Kamperis <ekamperi@gmail.com> 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

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<ekamperi@gmail.com> 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

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<ekamperi@gmail.com> 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

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

: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 
                    <dillon@backplane.com>

Fixed with:
http://gitweb.dragonflybsd.org/dragonfly.git/commit/e914c91d6da07b3c7540200e787d9a95159b611f