Re: CVS commit: src/lib/librumpuser

2020-04-01 Thread Kamil Rytarowski
On 01.04.2020 17:06, Robert Elz wrote:
> Date:Wed, 1 Apr 2020 15:54:15 +0200
> From:Kamil Rytarowski 
> Message-ID:  <969362d2-d93e-2cf4-7437-ab593ab11...@gmx.com>
>
>   | Ping? This still breaks.
>
> I am still working on it.Best I can tell at the minute is that the \0
> is potentially needed (in a theoretical sense) but not by anything
> operating rationally.
>
> That is, when rump is used the strings will already always be \0 terminated
> and the extra one added (the one that is off the end of the array) is never
> needed, there's always an earlier one.
>
> However, the relevant struct (that contains the string) comes from some
> other process, and while if that process is running rump code, which is
> what is intended to happen, all will be OK (I believe, I am not finished
> checking all of that code), if it is something else, generating rump
> packets, and passing them through, then we have no idea what will
> be there, and the \0 termination cannot be guaranteed (and if we don't
> do something, the rump process will eventually do bizarre things, that
> out of the array \0 is currently preventing that possibility).
>
> I see two reasonable paths forward here:
>
> 1. instead of adding the \0 off the end of the array, check that the
> array is already \0 terminated (it should be, and always is in the ATF
> test uses of rump - I ran the tests with a check in place, and it never
> failed) - the \0 is always in the final byte of the array (the one you
> overwrote in your earlier change, which meant that the changed line was
> just a no-op, in practice, as suspected earlier.)
>
> 2. When we are reading an exec rump struct, allocate (and zero - the zero
> part is already present) 1 byte more than will be received from the
> sending process, so that the final byte will always remain as a \0, and
> we will absolutely guarantee that the string will be \0 terminated (in
> all normal cases it would end up terminated by two \0's).
>
> If we do either of these, we don't need to waste time verifying that rump
> always does send (in every case) a \0 terminated string (digging through the
> code to work out where some of these structs get built is a slow process)
> as the actual problem will be solved either way.
>
> Solution 1 makes it an error, and the rup process will fail the exec if
> the path isn't correctly \0 terminated.   Solution 2 does what the code
> currently does (effectively) adding a \0 beyond the string that is received
> from the sending process, but does it within the array bounds (by making the
> array bigger) rather than outside them.
>
> Opinions for which is better?
>

Going for 2. is a little bit safer and we can reduce researching corner
cases that might never happen anyway.

> kre
>



Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Taylor R Campbell
> Date: Wed, 1 Apr 2020 07:42:53 -0700
> From: Jason Thorpe 
> 
> If PAGE_SIZE is ostensibly a vsize_t / size_t, why not define it as (1U << 
> PAGE_SHIFT)?

Without running the following program, can you tell me what it will
print?

It might work to define PAGE_SIZE to be ((size_t)1 << PAGE_SHIFT) but
the consequences are not immediately clear to me (and might be tricky
on, e.g., i386pae).


#include 
#include 
#include 

#define PAGE_SIZE_S (1 << 12)
#define PAGE_SIZE_U (1u << 12)

uint64_taddr = 0x7fff01234567;

int
main(void)
{

printf("%"PRIx64"\n", addr & ~(PAGE_SIZE_S - 1));
printf("%"PRIx64"\n", addr & ~(PAGE_SIZE_U - 1));

fflush(stdout);
return ferror(stdout);
}


Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Christos Zoulas
I think that PAGESIZE is not always a constant in our code.

christos




signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Robert Elz
Date:Wed, 1 Apr 2020 16:31:01 +0200
From:Kamil Rytarowski 
Message-ID:  

  | Does it look better?
  |
  | http://netbsd.org/~kamil/patch-00244-fopsmapper-PAGE_SIZE.txt

Apart from it needing to be (expressed in the relevant
make syntax, whatever that is)

if (WARNS > 3) WARNE=3

rather than simply WARNS=3 then as a hack type fix that looks fine.

But this is really a gcc bug, we shouldn't be needing to change our
sources at all because of this.

The signed vs unsigned comparison warning is because if the signed number
happens to be negative, when treated as unsigned it will compare larger
(usually) than the unsigned value - whereas most people expect negative
numbers to be less than positive ones (whatever the magnitudes).   Warning
for that potential problem is entirely reasonable.

But here we have a known positive (though signed) constant.   It can never
magically become negative, and so the problem can never exist.  There should
not (ever) be a warning for this case, it is simply stupid, and we should
not need to modify either code, or makefiles, to make it go away.

kre




Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Christos Zoulas

> If PAGE_SIZE is ostensibly a vsize_t / size_t, why not define it as (1U << 
> PAGE_SHIFT)?

It will *probably* work unless we have 'if (negative_int > PAGESIZE)'
somewhere. I guess if you make the change and the kernel boots... :-)

christos


signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/lib/librumpuser

2020-04-01 Thread Robert Elz
Date:Wed, 1 Apr 2020 15:54:15 +0200
From:Kamil Rytarowski 
Message-ID:  <969362d2-d93e-2cf4-7437-ab593ab11...@gmx.com>

  | Ping? This still breaks.

I am still working on it.Best I can tell at the minute is that the \0
is potentially needed (in a theoretical sense) but not by anything
operating rationally.

That is, when rump is used the strings will already always be \0 terminated
and the extra one added (the one that is off the end of the array) is never
needed, there's always an earlier one.

However, the relevant struct (that contains the string) comes from some
other process, and while if that process is running rump code, which is
what is intended to happen, all will be OK (I believe, I am not finished
checking all of that code), if it is something else, generating rump
packets, and passing them through, then we have no idea what will
be there, and the \0 termination cannot be guaranteed (and if we don't
do something, the rump process will eventually do bizarre things, that
out of the array \0 is currently preventing that possibility).

I see two reasonable paths forward here:

1. instead of adding the \0 off the end of the array, check that the
array is already \0 terminated (it should be, and always is in the ATF
test uses of rump - I ran the tests with a check in place, and it never
failed) - the \0 is always in the final byte of the array (the one you
overwrote in your earlier change, which meant that the changed line was
just a no-op, in practice, as suspected earlier.)

2. When we are reading an exec rump struct, allocate (and zero - the zero
part is already present) 1 byte more than will be received from the
sending process, so that the final byte will always remain as a \0, and
we will absolutely guarantee that the string will be \0 terminated (in
all normal cases it would end up terminated by two \0's).

If we do either of these, we don't need to waste time verifying that rump
always does send (in every case) a \0 terminated string (digging through the
code to work out where some of these structs get built is a slow process)
as the actual problem will be solved either way.

Solution 1 makes it an error, and the rup process will fail the exec if
the path isn't correctly \0 terminated.   Solution 2 does what the code
currently does (effectively) adding a \0 beyond the string that is received
from the sending process, but does it within the array bounds (by making the
array bigger) rather than outside them.

Opinions for which is better?

kre



Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Jason Thorpe


> On Apr 1, 2020, at 7:17 AM, Christos Zoulas  wrote:
> 
> Which we have been slowly fixing. I think in this case the sign-compare
> warnings are annoying, but putting casts on each warning is cluttering
> the code needlessly. Unfortunately the alternative (to make the PAGESIZE
> constant unsigned is more dangerous -- unless of course you are willing to 
> examine all the places it is used and make sure that the semantics don't
> change). Another way would be to make:
> 
>#define PAGESIZE_U ((unsigned)PAGESIZE)

If PAGE_SIZE is ostensibly a vsize_t / size_t, why not define it as (1U << 
PAGE_SHIFT)?

-- thorpej



Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Kamil Rytarowski
On 01.04.2020 16:17, Christos Zoulas wrote:
> In article ,
> Paul Goyette   wrote:
>> On Wed, 1 Apr 2020, Kamil Rytarowski wrote:
>>
>>> On 01.04.2020 15:47, Robert Elz wrote:
 Date:Wed, 1 Apr 2020 11:45:53 +
 From:"Kamil Rytarowski" 
 Message-ID:  <20200401114554.05167f...@cvs.netbsd.org>

   | Log Message:
   | Avoid comparison between signed and unsigned integer
   |
   | Cast PAGE_SIZE to size_t.

 This kind of pedantry is going way too far, PAGE_SIZE is a compile
 time constant (1 << PAGE_SHIFT) which is an int (and so signed,
 nominally) but one which is known to be positive.

>>>
>>> I got reports that certain ports no longer build due to:
>>>
>>> src/sys/modules/examples/fopsmapper/fopsmapper.c:118:11: error:
>>> comparison between signed and unsigned integer expressions
>>> [-Werror=sign-compare]
>>>  if (size != PAGE_SIZE)
>>>   ^~
>>> cc1: all warnings being treated as errors
>>
>>
>> There's a lot of modules that fail for this with WARNS=5 when being
>> built as loadable modules.
>>
>> That's why so many of the individual module Makefiles have explicit
>> WARNS=4.  (It seems that when modules are built-in as part of the
>> kernel, they are by default built with WARNS=4.)
>
> Which we have been slowly fixing. I think in this case the sign-compare
> warnings are annoying, but putting casts on each warning is cluttering
> the code needlessly. Unfortunately the alternative (to make the PAGESIZE
> constant unsigned is more dangerous -- unless of course you are willing to
> examine all the places it is used and make sure that the semantics don't
> change). Another way would be to make:
>
> #define PAGESIZE_U ((unsigned)PAGESIZE)
>
> and use that instead; eventually when all instances of PAGESIZE have been
> converted to PAGESIZE_U it can be removed. But is it worth it? There are
> perhaps better things to do. But anyway you want to proceed should be
> discussed in tech-kern. Adding piecemeal casts everywhere does not make
> the world a better place.
>
> christos
>

Does it look better?

http://netbsd.org/~kamil/patch-00244-fopsmapper-PAGE_SIZE.txt

Perhaps we could switch PAGE_SIZE to unsigned.. but it is too much for now.


Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Christos Zoulas
In article ,
Paul Goyette   wrote:
>On Wed, 1 Apr 2020, Kamil Rytarowski wrote:
>
>> On 01.04.2020 15:47, Robert Elz wrote:
>>> Date:Wed, 1 Apr 2020 11:45:53 +
>>> From:"Kamil Rytarowski" 
>>> Message-ID:  <20200401114554.05167f...@cvs.netbsd.org>
>>>
>>>   | Log Message:
>>>   | Avoid comparison between signed and unsigned integer
>>>   |
>>>   | Cast PAGE_SIZE to size_t.
>>>
>>> This kind of pedantry is going way too far, PAGE_SIZE is a compile
>>> time constant (1 << PAGE_SHIFT) which is an int (and so signed,
>>> nominally) but one which is known to be positive.
>>>
>>
>> I got reports that certain ports no longer build due to:
>>
>> src/sys/modules/examples/fopsmapper/fopsmapper.c:118:11: error:
>> comparison between signed and unsigned integer expressions
>> [-Werror=sign-compare]
>>  if (size != PAGE_SIZE)
>>   ^~
>> cc1: all warnings being treated as errors
>
>
>There's a lot of modules that fail for this with WARNS=5 when being
>built as loadable modules.
>
>That's why so many of the individual module Makefiles have explicit
>WARNS=4.  (It seems that when modules are built-in as part of the
>kernel, they are by default built with WARNS=4.)

Which we have been slowly fixing. I think in this case the sign-compare
warnings are annoying, but putting casts on each warning is cluttering
the code needlessly. Unfortunately the alternative (to make the PAGESIZE
constant unsigned is more dangerous -- unless of course you are willing to 
examine all the places it is used and make sure that the semantics don't
change). Another way would be to make:

#define PAGESIZE_U ((unsigned)PAGESIZE)

and use that instead; eventually when all instances of PAGESIZE have been
converted to PAGESIZE_U it can be removed. But is it worth it? There are
perhaps better things to do. But anyway you want to proceed should be
discussed in tech-kern. Adding piecemeal casts everywhere does not make
the world a better place.

christos



Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Kamil Rytarowski
On 01.04.2020 16:10, Paul Goyette wrote:
> On Wed, 1 Apr 2020, Kamil Rytarowski wrote:
>
>> On 01.04.2020 15:47, Robert Elz wrote:
>>>     Date:    Wed, 1 Apr 2020 11:45:53 +
>>>     From:    "Kamil Rytarowski" 
>>>     Message-ID:  <20200401114554.05167f...@cvs.netbsd.org>
>>>
>>>   | Log Message:
>>>   | Avoid comparison between signed and unsigned integer
>>>   |
>>>   | Cast PAGE_SIZE to size_t.
>>>
>>> This kind of pedantry is going way too far, PAGE_SIZE is a compile
>>> time constant (1 << PAGE_SHIFT) which is an int (and so signed,
>>> nominally) but one which is known to be positive.
>>>
>>
>> I got reports that certain ports no longer build due to:
>>
>> src/sys/modules/examples/fopsmapper/fopsmapper.c:118:11: error:
>> comparison between signed and unsigned integer expressions
>> [-Werror=sign-compare]
>>  if (size != PAGE_SIZE)
>>   ^~
>> cc1: all warnings being treated as errors
>
>
> There's a lot of modules that fail for this with WARNS=5 when being
> built as loadable modules.
>
> That's why so many of the individual module Makefiles have explicit
> WARNS=4.  (It seems that when modules are built-in as part of the
> kernel, they are by default built with WARNS=4.)
>

I tracked it to WARNS=3

# Rather than our usual WARNS=5, we need to use 3, since there are a
# lot of signed-vs-unsigned compares

WARNS=  3

Personally I have got no preference. I'm fine with an explicit cast as
it is in a single place only needed (hopefully).


Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Paul Goyette

On Wed, 1 Apr 2020, Kamil Rytarowski wrote:


On 01.04.2020 15:47, Robert Elz wrote:

Date:Wed, 1 Apr 2020 11:45:53 +
From:"Kamil Rytarowski" 
Message-ID:  <20200401114554.05167f...@cvs.netbsd.org>

  | Log Message:
  | Avoid comparison between signed and unsigned integer
  |
  | Cast PAGE_SIZE to size_t.

This kind of pedantry is going way too far, PAGE_SIZE is a compile
time constant (1 << PAGE_SHIFT) which is an int (and so signed,
nominally) but one which is known to be positive.



I got reports that certain ports no longer build due to:

src/sys/modules/examples/fopsmapper/fopsmapper.c:118:11: error:
comparison between signed and unsigned integer expressions
[-Werror=sign-compare]
 if (size != PAGE_SIZE)
  ^~
cc1: all warnings being treated as errors



There's a lot of modules that fail for this with WARNS=5 when being
built as loadable modules.

That's why so many of the individual module Makefiles have explicit
WARNS=4.  (It seems that when modules are built-in as part of the
kernel, they are by default built with WARNS=4.)



++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+


Re: CVS commit: src/lib/librumpuser

2020-04-01 Thread Kamil Rytarowski
On 24.03.2020 19:37, Kamil Rytarowski wrote:
> On 24.03.2020 15:41, Robert Elz wrote:
>> Date:Tue, 24 Mar 2020 13:27:45 +0100
>> From:Kamil Rytarowski 
>> Message-ID:  <5ec1195a-f1c8-cd46-6a14-ea29da109...@gmx.com>
>>
>>   | I patched it myself only when I reproduced the problems myself.
>>
>> I have no doubt that there's a bug that needs fixing - it is the fix
>> proposed that is wrong.   My guess is that most probably it is simply
>> doing nothing useful (no harm, no good either) but I need to confirm
>> that.   If it is, the correct fix is simply to delete the line (both
>> times it was changed).   If not, there's a more serious problem elsewhere
>> that needs fixing elsewhere (after which the line can be deleted!)
>>
>>   | OK. I will do it and please fix it in a better way.
>>
>> Working on it now.
>>
>
> Thanks.
>
>> I haven't seen the revert yet, when I do I will commit the fix (or a fix,
>> this one is somewhat debatble what is correct, though what is there now
>> is obviously wrong ... but just like the one in question, while wrong, it
>> is, in practice, harmless, at least in any normal use of librump).
>>
>> The fix for this issue needs to wait until the real problem the offending
>> line is there to deal with (if any, which I suspect is not the case) is
>> found.
>>
>
> ASan detects not a hypothetical, but factual momory corruption.
>
> I'm attaching a report from ASan.
>
> =
> ==11092==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x6020101e at pc 0x7f7ff6a10419 bp 0x7f7fe942fcb0 sp 0x7f7fe942fca8
> WRITE of size 1 at 0x6020101e thread T30
> #0 0x7f7ff6a10418 in handlereq
> /usr/src/lib/librumpuser/rumpuser_sp.c:984:18
> #1 0x7f7ff6a10418 in spserver
> /usr/src/lib/librumpuser/rumpuser_sp.c:1280:7
> #2 0x7f7ff660cf36 in pthread__create_tramp
> /usr/src/lib/libpthread/pthread.c:587:11
>
> 0x6020101e is located 0 bytes to the right of 14-byte region
> [0x60201010,0x6020101e)
> allocated by thread T30 here:
> #0 0x311793 in malloc (/usr/bin/rump_server+0x111793)
> #1 0x7f7ff6a0e5bb in readframe
> /usr/src/lib/librumpuser/sp_common.c:505:18
> #2 0x7f7ff6a0e5bb in spserver
> /usr/src/lib/librumpuser/rumpuser_sp.c:1268:13
> #3 0x7f7ff660cf36 in pthread__create_tramp
> /usr/src/lib/libpthread/pthread.c:587:11
>
> Thread T30 created by T0 here:
> #0 0x2e054d in pthread_create (/usr/bin/rump_server+0xe054d)
>
> SUMMARY: AddressSanitizer: heap-buffer-overflow
> /usr/src/lib/librumpuser/rumpuser_sp.c:984:18 in handlereq
> Shadow bytes around the buggy address:
>   0x4c0401b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x4c0401c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x4c0401d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x4c0401e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x4c0401f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> =>0x4c040200: fa fa 00[06]fa fa fa fa fa fa fa fa fa fa fa fa
>   0x4c040210: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x4c040220: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x4c040230: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x4c040240: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x4c040250: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> Shadow byte legend (one shadow byte represents 8 application bytes):
>   Addressable:   00
>   Partially addressable: 01 02 03 04 05 06 07
>   Heap left redzone:   fa
>   Freed heap region:   fd
>   Stack left redzone:  f1
>   Stack mid redzone:   f2
>   Stack right redzone: f3
>   Stack after return:  f5
>   Stack use after scope:   f8
>   Global redzone:  f9
>   Global init order:   f6
>   Poisoned by user:f7
>   Container overflow:  fc
>   Array cookie:ac
>   Intra object redzone:bb
>   ASan internal:   fe
>   Left alloca redzone: ca
>   Right alloca redzone:cb
>   Shadow gap:  cc
> ==11092==ABORTING
>
> Getting now more debug info is too time consuming (build times are
> excessive in my setup).
>

Ping? This still breaks.


Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Kamil Rytarowski
On 01.04.2020 15:47, Robert Elz wrote:
> Date:Wed, 1 Apr 2020 11:45:53 +
> From:"Kamil Rytarowski" 
> Message-ID:  <20200401114554.05167f...@cvs.netbsd.org>
>
>   | Log Message:
>   | Avoid comparison between signed and unsigned integer
>   |
>   | Cast PAGE_SIZE to size_t.
>
> This kind of pedantry is going way too far, PAGE_SIZE is a compile
> time constant (1 << PAGE_SHIFT) which is an int (and so signed,
> nominally) but one which is known to be positive.
>

I got reports that certain ports no longer build due to:

src/sys/modules/examples/fopsmapper/fopsmapper.c:118:11: error:
comparison between signed and unsigned integer expressions
[-Werror=sign-compare]
  if (size != PAGE_SIZE)
   ^~
cc1: all warnings being treated as errors


Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Robert Elz
Date:Wed, 1 Apr 2020 11:45:53 +
From:"Kamil Rytarowski" 
Message-ID:  <20200401114554.05167f...@cvs.netbsd.org>

  | Log Message:
  | Avoid comparison between signed and unsigned integer
  |
  | Cast PAGE_SIZE to size_t.

This kind of pedantry is going way too far, PAGE_SIZE is a compile
time constant (1 << PAGE_SHIFT) which is an int (and so signed,
nominally) but one which is known to be positive.

What is to be next?  Given an unsigned var (any unsigned type) 'u'
are we going to be required to write

if (u != 0U)
or
if (u != (unsigned)0)

instead of just
if (u != 0)
?

Get rid of the cast, it isn't needed in this case,
and anything that believes it is, is wrong (broken).

kre