Re: CVS commit: src/lib/librumpuser

2020-05-06 Thread Alistair Crooks
On Tue, 5 May 2020 at 20:54, Kamil Rytarowski  wrote:

> Ping? We are blocked by this in GSoC now.
>
>
> I doubt that you are "blocked by this in GSoC", as the GSoC projects were
just announced yesterday.

You should be planning the milestones right now with your student


Re: CVS commit: src/lib/librumpuser

2020-05-06 Thread Robert Elz
Date:Wed, 6 May 2020 07:30:17 +0200
From:Kamil Rytarowski 
Message-ID:  

  | I reverted my fix

It wasn't a fix, it simply made the problem go away, incorrectly.

If you want you can just delete the relevant lines (the ones you changed).
I've been running like that now since we started talking about this, and
everything appears fine.   You can put an  "OK kre" on the commit if you
do that.

I believe more changes are (or might be) needed - but that will correct
the buffer overflow problem, without breaking anything that will be visible
(and perhaps, nothing at all, I am still not certain).

  | from Match with a promise to see a better fix by kre@
  | but it never happened.

Yes, I got a bit side tracked with other issues, but this one is not
forgotten, just temporarily pushed down the stack a little.   It was
back near the top of the stack again, even before this small exchange
of messages.

kre



Re: CVS commit: src/lib/librumpuser

2020-05-05 Thread Kamil Rytarowski
On 06.05.2020 07:17, Alistair Crooks wrote:
> 
> 
> On Tue, 5 May 2020 at 20:54, Kamil Rytarowski  > wrote:
> 
> Ping? We are blocked by this in GSoC now.
> 
> 
> I doubt that you are "blocked by this in GSoC", as the GSoC projects
> were just announced yesterday.
> 
> You should be planning the milestones right now with your student

We already worked on preparation to this GSoC in the past few months.

I reverted my fix from Match with a promise to see a better fix by kre@
but it never happened. This bug breaks rumpkernel usage with ASan (and
possibly MSan/TSan which weren't tested with rump). Lack of fix blocks
two selected GSoC tasks (or forces us to keep local patches).



signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/lib/librumpuser

2020-05-05 Thread Kamil Rytarowski
Ping? We are blocked by this in GSoC now.

On 01.04.2020 20:19, Kamil Rytarowski wrote:
> 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
>>
> 




signature.asc
Description: OpenPGP digital signature


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/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/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/lib/librumpuser

2020-03-25 Thread Robert Elz
Date:Tue, 24 Mar 2020 19:37:02 +0100
From:Kamil Rytarowski 
Message-ID:  <57a7e062-9af0-0be9-cb24-e155c5f83...@gmx.com>

  | ASan detects not a hypothetical, but factual momory corruption.

Yes, I know that - and I believed you from the start that there was a
buffer overrun there.

The issue is whether the offending line was useful for anything or not.

I am currently running ATF tests with that line simply deleted (actually,
replaced by a check that there is a \0 in the buffer somewhere already,
and abort() if not).

I'm expecting, for our ATF tests, that this is going to work fine (not abort),
which is why your "fix" looked correct - it certainly avoided the buffer
overrun, and was simply writing a \0 either on top of one that already was
present in the same byte (I am going to do another check to see if this was
the case next, but your asan output from this message suggests probably not)
or after an earlier \0 somewhere previously in the buffer (ie: in empty
unused space beyond the end of the string (which the asan output suggests
is the more likely case)).

Note that knowing that this is true of our tests, while useful info, proves
nothing - different data might stress the code in different ways, hence I
am going to find where (everywhere) the data is first constructed, and check
that it always guarantees \0 termination.   If there is, then the total
fix for the ASAN problem is to delete the offending line.   If not, the
correct fix is to make sure that the data is always correctly \0 terminated
-- and then delete the offending line.That's what I mean about "wait for
the real problem" - we need to find out whether the line that wrote beyond
the buffer end was there merely as a "I am going to treat this data like a
string, and bad things will happen if it isn't \0 terminated somehow, so
I will just stick a \0 after the buffer, just in case" type protection
mechanism, that was never really needed in the first place (a security
blanket), or whether there is some case where the code, given appropriate
data (say an exec that is MAXPATHLEN long, or something ... this is just
wild speculation of course) where it might currently be possible that no \0
is present, in which case that \0 added was saving things. and your fix was 
corrupting the data, and the correct fix is to make the buffer 1 byte bigger
so the \0 will fit (where the data is originally constructed).

We just don't know yet - and no amount of random testing will tell us (except
by fluke, and even that would just indicate that there is a real problem,
by managing to use data that triggers it, but probably not where in the code
is the base cause), it needs careful code reading.

This is why when the sanitisers find a problem, and it isn't obvious what
is the real cause (as opposed to the actual line that causes the problem)
you should avoid making random "fixes" to make the sanitiser report go away
(by no longer doing whatever it is complaining about - but not necessarily
implementing the original algorithm, whatever it was, correctly either).

This is an example of a case like that.   On the other hand, the other change
you committed at about the same time (the one with the iov where there was
an a && b test, where logically both have to be true for the code to be
executed, so it shouldn't matter which order, so programmers tend to put the
more likely to be false first, to save testing the less likely one when the
other is false - but in the case in questionb, when "b" was false, testing
"a" was accessing beyond the end of the memory.   Ie: the test should have
been b && a - and that's what you changted it to.   For that one, the cause
of the issue was obvious, and the fix correct - when you see ones that are
that simple, by all means, just fix them, as you did that time.

But if in doubt, file a PR - sanitiser detected bugs, while (at least often)
real bugs (ubsan not quite so much...) are rarely, if ever, critical fix
type - they have usually been present for years, harming no-one, so taking
a few extra days/weeks/months to arrive at the correct fix isn't really
doing much harm, usually.

kre



Re: CVS commit: src/lib/librumpuser

2020-03-24 Thread Kamil Rytarowski
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).



signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/lib/librumpuser

2020-03-24 Thread Robert Elz
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.

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.

kre



Re: CVS commit: src/lib/librumpuser

2020-03-24 Thread Kamil Rytarowski
On 24.03.2020 06:09, Robert Elz wrote:
> Date:Tue, 24 Mar 2020 05:40:13 +0100
> From:Kamil Rytarowski 
> Message-ID:  
> 
> 
>   | This patch was sitting in the tree since August 2019.
> 
> In your tree I assume you mean, it certainly hasn't been in mine.
> 

A similar fix was done by shm@ with his public repo in August last year.

https://github.com/LogicalTrust/fuzzrump-src/commit/f5d2fb34ea597bb92a72d31236ad2f350a2fd8be

That work was not secret as it was presented in at least 2 public and
crowded conference. As far as I recall it was not reported as PR or on a
ML before.

I patched it myself only when I reproduced the problems myself.

> OK, then please revert that change (which cannot be the right fix, regardless
> of what the right one really is) and file a PR, so it can be fixed properly.
> 

OK. I will do it and please fix it in a better way.

> kre
> 
> ps: while looking at this I spotted a (complely unrelated) bug in the same
> file that should be fixed (no, sanitisers will never find this one, but
> human reading easily might) - which I will fix, but I'd prefer to do that
> after this fix is reverted, just so those two commit messages are adjacent.
> 

Thank you in advance!



signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/lib/librumpuser

2020-03-23 Thread Robert Elz
Date:Tue, 24 Mar 2020 05:40:13 +0100
From:Kamil Rytarowski 
Message-ID:  


  | This patch was sitting in the tree since August 2019.

In your tree I assume you mean, it certainly hasn't been in mine.

Was a PR filed about the issue back then?   If so, shouldn't it have
been referenced in the commit message?If not, one should have been.

If that had (either initially, or in a later add on message) included
the suggested patch, I (and probably several others) would have told you
at the time it was not the correct thing to do - it took all of 30 seconds
of looking at the code to know that it is impossible to be the right fix.

  | This patch was tested to be operational.

I am not surprised at that at all - like I said (but I have still not
analysed things fully) I suspect that all it is doing is writing one \0
on top of a \0 that is already there, and that the correct patch would
have been to simply delete the offending line (and the comments attached
to it).

But that's still a guess.

  | I am aware that writing out of allocated buffer is for some people
  | considered as 'no bug'.

That's not what I said.   What I said was that while it is a bug, and
should be fixed, it was evidently harmless most of the time, as everything
has been working with this bug in it.

There are several possible problems that might exist here, perhaps (maybe
only in some extreme case) the buffer allocated is one byte too short, and
the correct fix would be to add a "+1" to some malloc() call elsewhere in
the code (I doubt that one, but it is possible).

Or perhaps the code that calculates commlen is incorrect, and it should
have had a "-1" included in the expression.

Or perhaps some code that is copying in the args is stopping just before
the \0 that terminates eact string, and failing to copy that one where it
should be, and this extra \0 appended is essential (in which case a correct
fix might be to copy the data into a slightly bigger buffer where there is
space for a \0 to be appended ... or change the arg copying code to include
the \0 rather than exclude it).   I doubt this one too, as I doubt that
things would have worked if this was the case - and your "it works with
the patch" makes it even less likely that this is the problem.

Or perhaps something else.

The point is that this needs to be analysed correctly, not just hacked at
to fix the problem, and that the line of code as it is now is either entirely
reduncant (my guess) or (perhaps only sometimes - and perhaps never in our
current tests) breaks things.

  | I'm fine to see it fixed in a better way than manually injected \0.

OK, then please revert that change (which cannot be the right fix, regardless
of what the right one really is) and file a PR, so it can be fixed properly.

kre

ps: while looking at this I spotted a (complely unrelated) bug in the same
file that should be fixed (no, sanitisers will never find this one, but
human reading easily might) - which I will fix, but I'd prefer to do that
after this fix is reverted, just so those two commit messages are adjacent.



Re: CVS commit: src/lib/librumpuser

2020-03-23 Thread Kamil Rytarowski
On 24.03.2020 05:27, Robert Elz wrote:
> There was no great urgency to "fix"
> this particular one, it doesn't seem to have been causing any real problems,
> and could have easily waited a few days (or weeks, or even months) until
> the correct solution was found.
> 
> kre
> 

This patch was sitting in the tree since August 2019.

This patch was tested to be operational.

I am aware that writing out of allocated buffer is for some people
considered as 'no bug'.

I'm fine to see it fixed in a better way than manually injected \0.



signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/lib/librumpuser

2020-03-23 Thread Robert Elz
Date:Tue, 24 Mar 2020 01:56:56 +
From:"Kamil Rytarowski" 
Message-ID:  <20200324015656.33df1f...@cvs.netbsd.org>

  | Module Name:src
  | Committed By:   kamil
  | Date:   Tue Mar 24 01:56:56 UTC 2020
  |
  | Modified Files:
  | src/lib/librumpuser: rumpuser_sp.c
  |
  | Log Message:
  | Avoid buffer overflow
  |
  | Detected with ASan + RUMPKERNEL.

This "fix" cannot possibly be correct.

The code was (twice, the two cases are the same, so consider
just one of them)

/* ensure comm is 0-terminated */  
/* TODO: make sure it contains sensible chars? */
comm[commlen] = '\0';

where the "fix" is for the final line to be changed into

comm[commlen-1] = '\0';

Now I can see that the original may have been writing (perhaps just
sometimes, I haven't analysed it fully) one byte beyond the end of
the allocated space, and so something should be fixed, but your
change cannot possibly be the right way.

There are two possibilities here, in any particular case, either comm
was already nul-terminated, in which case the assignment isn't needed
at all, or it wasn't, in which case the changed code just destroyed the
final data byte by dumping a \0 on it - turning what was most probably
a harmless (though technically broken) one byte buffer overrun into
guaranteed broken code.

My guess is that the buffer is (always) already nul-terminated, and
the assignment was just paranoid over protection - and that the change
that was made simply overwrites one \0 with a different \0 (and so is
harmless, but stupid, the correct fix would be to delete the assignment
completely, similarly the "TODO" is nonsense, whatever the app decided
to exec() is its business, 'sensible' chars or not - either it works, or
doesn't, it isn't rump's business to second guess the client code).

I assume that because for exec() (which is what this is handling), the
arg strings are all \0 terminated - detecting that \0 is the only way the
arg copyin code knows where to stop.

If my guess is wrong, then the bug is where the args are collected. and
should be fixed there, not by plonking a \0 in here.

Kamil, once again, as I have asked before - if you don't have time to
fully analyse the problems that the sanitisers detect, please DO NOT
"fix" them - just file a PR and let someone who has the time deal with
it.Making the code generate no sanitiser bug reports by breaking it
is not our objective (I hope).   There was no great urgency to "fix"
this particular one, it doesn't seem to have been causing any real problems,
and could have easily waited a few days (or weeks, or even months) until
the correct solution was found.

kre



Re: CVS commit: src/lib/librumpuser/build-aux

2016-10-30 Thread coypu
For the record, this is because the build server is running -6-1..


Re: CVS commit: src/lib/librumpuser/build-aux

2016-10-19 Thread coypu
On Wed, Oct 19, 2016 at 09:41:30AM +, Antti Kantee wrote:
> No idea about the cause, but there seem to be other variants of the same
> theme in the build logs, suggesting a more general problem, e.g.:
> https://releng.netbsd.org/builds/HEAD/201610171520Z/sbmips-mipseb.build.failed
> 
> In all of the cases I looked at, it was a lib, but don't know if that holds
> universally.
> 

Yes. there are many occurences of this script.
If it had been the cause, that would still make sense.

I reverted my change.


Re: CVS commit: src/lib/librumpuser/build-aux

2016-10-19 Thread Antti Kantee

On 19/10/16 06:05, co...@sdf.org wrote:

Maybe I should revert it because I misunderstood things and it does not
matter... sorry for the noise


Yes.


I was trying to do something about the random failures in builds

http://releng.netbsd.org/builds/netbsd-6/201610160430Z/ews4800mips.build.failed
http://releng.netbsd.org/builds/netbsd-6/201610181650Z/sbmips-mipsel.build.failed


No idea about the cause, but there seem to be other variants of the same 
theme in the build logs, suggesting a more general problem, e.g.:

https://releng.netbsd.org/builds/HEAD/201610171520Z/sbmips-mipseb.build.failed

In all of the cases I looked at, it was a lib, but don't know if that 
holds universally.


thanks,
  antti


Re: CVS commit: src/lib/librumpuser/build-aux

2016-10-19 Thread coypu
On Tue, Oct 18, 2016 at 10:28:04PM +, Antti Kantee wrote:
> On 17/10/16 18:24, Maya Rashish wrote:
> >Module Name: src
> >Committed By:maya
> >Date:Mon Oct 17 18:24:42 UTC 2016
> >
> >Modified Files:
> > src/lib/librumpuser/build-aux: install-sh
> >
> >Log Message:
> >use mktemp instead of $RANDOM for tmpdir
> >
> >..$RANDOM won't work with our /bin/sh.
> >
> >unsure if this script is used, but it is wrong.
> >might help the spurious build failures that occasionally
> >show up on autobuilds.
> 
> Are you planning to adjust all of the unused install-sh files around the
> tree to make them consistently less wrong?

Maybe I should revert it because I misunderstood things and it does not
matter... sorry for the noise

I was trying to do something about the random failures in builds

http://releng.netbsd.org/builds/netbsd-6/201610160430Z/ews4800mips.build.failed
http://releng.netbsd.org/builds/netbsd-6/201610181650Z/sbmips-mipsel.build.failed


Re: CVS commit: src/lib/librumpuser/build-aux

2016-10-18 Thread Antti Kantee

On 17/10/16 18:24, Maya Rashish wrote:

Module Name:src
Committed By:   maya
Date:   Mon Oct 17 18:24:42 UTC 2016

Modified Files:
src/lib/librumpuser/build-aux: install-sh

Log Message:
use mktemp instead of $RANDOM for tmpdir

..$RANDOM won't work with our /bin/sh.

unsure if this script is used, but it is wrong.
might help the spurious build failures that occasionally
show up on autobuilds.


Are you planning to adjust all of the unused install-sh files around the 
tree to make them consistently less wrong?


Re: CVS commit: src/lib/librumpuser/build-aux

2016-10-18 Thread coypu
On Mon, Oct 17, 2016 at 06:24:42PM +, Maya Rashish wrote:
> Module Name:  src
> Committed By: maya
> Date: Mon Oct 17 18:24:42 UTC 2016
> 
> Modified Files:
>   src/lib/librumpuser/build-aux: install-sh
> 
> Log Message:
> use mktemp instead of $RANDOM for tmpdir
> 

Woops, this is totally redundant, but harmless...


Re: CVS commit: src/lib/librumpuser

2014-10-07 Thread Marc Balmer

Am 29.09.2014 um 17:54 schrieb Justin Cormack jus...@netbsd.org:

 Module Name:  src
 Committed By: justin
 Date: Mon Sep 29 15:54:28 UTC 2014
 
 Modified Files:
   src/lib/librumpuser: rumpuser_port.h
 
 Log Message:
 Minix also has getenv_r support
 
 
 To generate a diff of this commit:
 cvs rdiff -u -r1.34 -r1.35 src/lib/librumpuser/rumpuser_port.h
 
 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.
 
 Modified files:
 
 Index: src/lib/librumpuser/rumpuser_port.h
 diff -u src/lib/librumpuser/rumpuser_port.h:1.34 
 src/lib/librumpuser/rumpuser_port.h:1.35
 --- src/lib/librumpuser/rumpuser_port.h:1.34  Tue Jul 22 22:41:58 2014
 +++ src/lib/librumpuser/rumpuser_port.h   Mon Sep 29 15:54:28 2014
 @@ -1,4 +1,4 @@
 -/*   $NetBSD: rumpuser_port.h,v 1.34 2014/07/22 22:41:58 justin Exp $
 */
 +/*   $NetBSD: rumpuser_port.h,v 1.35 2014/09/29 15:54:28 justin Exp $
 */
 
 /*
  * Portability header for non-NetBSD platforms.
 @@ -112,8 +112,8 @@ clock_gettime(clockid_t clk, struct time
 #include sys/types.h
 #include sys/param.h
 
 -/* NetBSD is the only(?) platform with getenv_r() */
 -#if !defined(__NetBSD__)
 +/* NetBSD is almost the only platform with getenv_r() */
 +#if !(defined(__NetBSD__) || defined(__minix__))

Minix defines __minix, not __minix__, though.

 #include errno.h
 #include stdlib.h
 #include string.h
 



Re: CVS commit: src/lib/librumpuser

2014-10-07 Thread Justin Cormack
On Tue, Oct 7, 2014 at 10:19 AM, Marc Balmer m...@msys.ch wrote:
 -/* NetBSD is the only(?) platform with getenv_r() */
 -#if !defined(__NetBSD__)
 +/* NetBSD is almost the only platform with getenv_r() */
 +#if !(defined(__NetBSD__) || defined(__minix__))

 Minix defines __minix, not __minix__, though.

My compiler (cross compiler from git head recently) defines both, but
you are right the standard one used seems to be __minix...

Justin


Re: CVS commit: src/lib/librumpuser

2014-07-23 Thread Alexander Nasonov
Justin Cormack wrote:
 Module Name:  src
 Committed By: justin
 Date: Tue Jul 22 22:41:58 UTC 2014
 
 Modified Files:
   src/lib/librumpuser: Makefile rumpfiber.c rumpuser.c rumpuser_int.h
   rumpuser_port.h
 Added Files:
   src/lib/librumpuser: rumpuser_random.c
 
 Log Message:
 Clean up random implementation for librumpuser
 
 Use /dev/urandom for platforms without arc4random, not srandom(),
 deduplicate code, do not read excessive random bytes
 
 Reviewed by pooka@
 
 ...
 +int
 +rumpuser__random_init(void)
 +{
 +
 + random_fd = open(random_device, O_RDONLY);
 + if (random_fd  0) {
 + fprintf(stderr, random init open failed\n);
 + return 1;
 + }

return -1 ?

 ...

  #else
  #define ET(_v_) return (_v_) ? rumpuser__errtrans(_v_) : 0;
  #endif

 + rv = read(random_fd, buf, buflen  random_maxread ? random_maxread : 
 buflen);
 + if (rv  0) {
 + ET(errno);
 + }

I know it's not your code and it's not touched by your change but
ET(errno) looks a bit cryptic. If it appeared as 'return ERRTRANS(errno)',
that would significantly increase readability.

ET(0) can be changed to 'return 0'.

Alex


Re: CVS commit: src/lib/librumpuser

2014-06-17 Thread Alan Barrett

On Mon, 16 Jun 2014, Alexander Nasonov wrote:

Log Message:
Add __RCSID.



+#if !defined(lint)
+__RCSID($NetBSD: rumpuser_bio.c,v 1.8 2014/06/16 21:07:28 alnsn Exp $);
+#endif /* !lint */


Some historical uses of __RCSID have an unnecessary #if/#endif wrapper
around them, but for new uses, please just write __RCSID(...);
without any #if/#endif wrapper.

--apb (Alan Barrett)


Re: CVS commit: src/lib/librumpuser

2014-06-17 Thread Alexander Nasonov
Alan Barrett wrote:
 Some historical uses of __RCSID have an unnecessary #if/#endif wrapper
 around them, but for new uses, please just write __RCSID(...);
 without any #if/#endif wrapper.

I copy/pasted this block from another file from the same directory.

Alex


Re: CVS commit: src/lib/librumpuser

2014-06-17 Thread Antti Kantee
On Tue, Jun 17, 2014 at 08:42:35AM +, Alexander Nasonov wrote:
 Module Name:  src
 Committed By: alnsn
 Date: Tue Jun 17 08:42:35 UTC 2014
 
 Modified Files:
   src/lib/librumpuser: Makefile
 
 Log Message:
 Antti objected to including rumpuser_sync_icache. Exclude it from the build.

To be clear: the objection was to modifying a stable interface
without coordination.  The hypercall interface is implemented in
multiple places outside of the NetBSD tree, including by 3rd parties.

Think of it like changing the libc ABI: no matter the merits of
the change itself (which they too remain to be discussed), it will
be objected to.


Re: CVS commit: src/lib/librumpuser

2014-06-17 Thread Alexander Nasonov
Antti Kantee wrote:
 To be clear: the objection was to modifying a stable interface
 without coordination.  The hypercall interface is implemented in
 multiple places outside of the NetBSD tree, including by 3rd parties.

Stable interface in -current? How are you supposed to make any
significant changes then? 

As I stated in the private email to Antti, we need to support two
versions: stable for users and current for developers. Otherwise,
it will block a development of NetBSD if every time you need to
make a change in NetBSD-current, you have to wait for a rumpuser
bump for stable users even though many of them are outside of
NetBSD.

In other words, don't rely on NetBSD-current being 100% stable,
especially for users outside of NetBSD.

I don't know enough details about rump to be certain but would
splitting only rumpuser into two versions work? If there were two
versions of rumpuser I could tweak the makefile to build cpufunc.c
only if MKSLJIT=yes and it would solve the problem because MKSLJIT
is no by default on arm.

Another reason for splitting rumpuser is because some hypercalls can't
be implemented with POSIX API. I don't think we need to support Linux or
Solaris in NetBSD tree, especailly if it depends on OS-specific code.

 Think of it like changing the libc ABI: no matter the merits of
 the change itself (which they too remain to be discussed), it will
 be objected to.

I got an impression from your private email yesterday that the
approach is correct (you didn't say that librumpuser code must be
POSIX while the code I sent to you wasn't as it used NetBSD specific
sysarch syscall).

And because it was a pure addition of a new function and it didn't break
NetBSD build, I assumed that it's safe to add it. The only thing I
didn't do was rumpuser ABI version bump.

Alex


Re: CVS commit: src/lib/librumpuser

2014-06-17 Thread Antti Kantee

On 17/06/14 09:46, Alexander Nasonov wrote:

Antti Kantee wrote:

To be clear: the objection was to modifying a stable interface
without coordination.  The hypercall interface is implemented in
multiple places outside of the NetBSD tree, including by 3rd parties.


Stable interface in -current? How are you supposed to make any
significant changes then?



As I stated in the private email to Antti, we need to support two
versions: stable for users and current for developers. Otherwise,
it will block a development of NetBSD if every time you need to
make a change in NetBSD-current, you have to wait for a rumpuser
bump for stable users even though many of them are outside of
NetBSD.



In other words, don't rely on NetBSD-current being 100% stable,
especially for users outside of NetBSD.

I don't know enough details about rump to be certain but would
splitting only rumpuser into two versions work? If there were two
versions of rumpuser I could tweak the makefile to build cpufunc.c
only if MKSLJIT=yes and it would solve the problem because MKSLJIT
is no by default on arm.


I think that will be overengineering it, but if you want to come up with 
a concrete proposal patch, please.  I'd simply use discussion and not 
rushing commits to avoid issues here.


If you don't have time to wait for discussion or coordination, do 
everything in the privacy of the sljit component.


In either case, there's no need for the blocking development drama card.


Another reason for splitting rumpuser is because some hypercalls can't
be implemented with POSIX API. I don't think we need to support Linux or
Solaris in NetBSD tree, especailly if it depends on OS-specific code.


I don't think we need to support Linux or Solaris in the NetBSD tree. 
Similarly, I don't think we need to make changes which _unnecessarily_ 
makes supporting them hard.



Think of it like changing the libc ABI: no matter the merits of
the change itself (which they too remain to be discussed), it will
be objected to.


I got an impression from your private email yesterday that the
approach is correct (you didn't say that librumpuser code must be
POSIX while the code I sent to you wasn't as it used NetBSD specific
sysarch syscall).


For reference, here's what I wrote:

=== snip ===
If the problem is syncing icache, the approach is correct.

However, I'd argue that the problem is dynamically loading code into a 
rump kernel, and with that the interface falls somewhat short.  What if 
someone wants W^X?

=== snip ===

If you understood that as go ahead, sorry for not being clear, falls 
short was the comment that I was trying to ease in.


I'm not saying that librumpuser must be POSIX.  I'm not sure where 
you're getting that from.


What I _am_ saying is that there must be some critical thought going in 
to the interfaces and their implications.  We're several years past the 
oh I'll just add this because I need it today stage of discovery.



And because it was a pure addition of a new function and it didn't break
NetBSD build, I assumed that it's safe to add it. The only thing I
didn't do was rumpuser ABI version bump.


It's a pure addition in the same sense as if you made a pure addition to 
NetBSD's Xen hypercall interface and made guests unconditionally use it. 
 It would not break the NetBSD build.  Would you assume that's a safe 
change to make?


Re: CVS commit: src/lib/librumpuser

2014-06-17 Thread Alexander Nasonov
Antti Kantee wrote:
 I think that will be overengineering it, but if you want to come up with a
 concrete proposal patch, please.  I'd simply use discussion and not rushing
 commits to avoid issues here.

The code is in the tree already. I don't need anything else for
sljit. The sljit library doesn't support W^X protection. Rumpuser
has a way to create an executable page via rumpuser_anonmap(...
exec=true). I understand it's not perfect but it's not my area to
change memory management in rumpuser.

 If you don't have time to wait for discussion or coordination, do everything
 in the privacy of the sljit component.

Please teach me how to create a private component.

 In either case, there's no need for the blocking development drama card.

It's not a drama, it's a technical issue.

 ...

 For reference, here's what I wrote:
 
 === snip ===
 If the problem is syncing icache, the approach is correct.
 
 However, I'd argue that the problem is dynamically loading code into a rump
 kernel, and with that the interface falls somewhat short.  What if someone
 wants W^X?
 === snip ===
 
 If you understood that as go ahead, sorry for not being clear, falls
 short was the comment that I was trying to ease in.

My problem is syncing icache, therefore, the approach is correct.

 I'm not saying that librumpuser must be POSIX.  I'm not sure where you're
 getting that from.

rumpuser(3):

DESCRIPTION
 The rumpuser hypercall interfaces allow a rump kernel to access host
 resources.  A hypervisor implementation must implement the routines
 described in this document to allow a rump kernel to run on the host.
 The implementation included in NetBSD is for POSIX hosts.
  ^
  ^

and it indeed broke buildrump.sh builds on Linux because sysarch
stuff isn't available.

There is no way I can make this interface POSIX-compatible because
POSIX doesn't specify icache sync as far as I know.

 What I _am_ saying is that there must be some critical thought going in to
 the interfaces and their implications.  We're several years past the oh
 I'll just add this because I need it today stage of discovery.

Ok, I'll leave it to you to think about it. All I need now is a private
component for my change.

 ...

 It's a pure addition in the same sense as if you made a pure addition to
 NetBSD's Xen hypercall interface and made guests unconditionally use it.  It
 would not break the NetBSD build.  Would you assume that's a safe change to
 make?

In my case, I could easily build icache in rumpkern conditionally
with MKSLJIT if librumpuser didn't break on non NetBSD hosts.

Alex


Re: CVS commit: src/lib/librumpuser

2014-06-17 Thread Alexander Nasonov
Antti Kantee wrote:
 Use RUMPCOMP_USER_SRCS, several examples under src/sys/rump
Thanks for the pointer, will do.

 That's one more indication that sync icache is the wrong level of problem
 to represent at the interface level.

Existence of __clear_cache is an indication of the opposite.

I'm not saying that POSIX should have icache sync routine, they
could specify that mprotect syscall that turns on PROT_EXEC should
sync icache. But that's no guaranteed by POSIX.

 If it were a high-level, holistic
 interface, both the caller and callee would what needs to be done, and the
 caller could perhaps implement the same with a alternative method.  A
 low-level interface like sync icache for this memory range leaves no room
 for interpretation, even if it will ever be used for only one purpose.

Not quite sure what you're saying because no room for interpretation
is a good thing in general and using something for one purpose is even
better ;-)

-- 
Alex


Re: CVS commit: src/lib/librumpuser

2014-06-17 Thread Antti Kantee

On 17/06/14 11:23, Alexander Nasonov wrote:

If you don't have time to wait for discussion or coordination, do everything
in the privacy of the sljit component.


Please teach me how to create a private component.


Use RUMPCOMP_USER_SRCS, several examples under src/sys/rump


I'm not saying that librumpuser must be POSIX.  I'm not sure where you're
getting that from.


rumpuser(3):

DESCRIPTION
  The rumpuser hypercall interfaces allow a rump kernel to access host
  resources.  A hypervisor implementation must implement the routines
  described in this document to allow a rump kernel to run on the host.
  The implementation included in NetBSD is for POSIX hosts.
   ^
   ^


ic.  I'd change that to POSIX-like, but currently I cannot access nbcvs.


and it indeed broke buildrump.sh builds on Linux because sysarch
stuff isn't available.

There is no way I can make this interface POSIX-compatible because
POSIX doesn't specify icache sync as far as I know.


That's one more indication that sync icache is the wrong level of 
problem to represent at the interface level.  If it were a high-level, 
holistic interface, both the caller and callee would what needs to be 
done, and the caller could perhaps implement the same with a alternative 
method.  A low-level interface like sync icache for this memory range 
leaves no room for interpretation, even if it will ever be used for only 
one purpose.


Re: CVS commit: src/lib/librumpuser

2014-06-17 Thread Antti Kantee

On 17/06/14 11:52, Alexander Nasonov wrote:

That's one more indication that sync icache is the wrong level of problem
to represent at the interface level.


Existence of __clear_cache is an indication of the opposite.


Let's be thankful we're not discussing implementing a compiler.


If it were a high-level, holistic
interface, both the caller and callee would what needs to be done, and the
caller could perhaps implement the same with a alternative method.  A
low-level interface like sync icache for this memory range leaves no room
for interpretation, even if it will ever be used for only one purpose.


Not quite sure what you're saying because no room for interpretation
is a good thing in general and using something for one purpose is even
better ;-)


Ok, one more try, this time with an example:

* fact: interface will be used to say I have loaded code here, please 
arrange things so that it can be executed
* fact: you want to call the interface sync icache and possess those 
semantics


the example:
Let's assume some fictional platform where you need to sign newly loaded 
code before it can execute.  Should the implementation of sync icache 
on that platform sign code?  If not, you can't execute the code, which 
was the original purpose.  If yes, you've failed to implement the 
interface correctly, because that might not be what the caller wants at 
all.  Will such a platform be supported?  Who knows.  Is it cause to 
leave known problems into the interface?  Definitely not!


If you don't want to solve anything except your immediate problem, 
that's more than fair enough (we've all been there), but it needs to be 
done without breaking things for everyone else or knowingly introducing 
suboptimal interfaces that everyone else will suffer from.


Re: CVS commit: src/lib/librumpuser

2014-06-17 Thread Alexander Nasonov
Antti Kantee wrote:
 Ok, one more try, this time with an example:
 
 * fact: interface will be used to say I have loaded code here, please
 arrange things so that it can be executed
 * fact: you want to call the interface sync icache and possess those
 semantics
 
 the example:
 Let's assume some fictional platform where you need to sign newly loaded
 code before it can execute.  Should the implementation of sync icache on
 that platform sign code?  If not, you can't execute the code, which was the
 original purpose.  If yes, you've failed to implement the interface
 correctly, because that might not be what the caller wants at all.  Will
 such a platform be supported?  Who knows.  Is it cause to leave known
 problems into the interface?  Definitely not!

You're over generalizing. You can't generate and sign your own code
without posing a security risk. This case deserves a special interface,
you can't just bring it under umbrella of a single hypercall that does
everything to turn memory into code.

I'm following a common practice of calling __clear_cache for JIT code
except that I wrap it as a hypercall. 

 If you don't want to solve anything except your immediate problem, that's
 more than fair enough (we've all been there),

We've seen many examples of over generalizing too.

My need is driven by existing sljit code which follows a common practice
of calling __clear_cache() gcc extension. If you want to generalize it,
we will need to work with sljit upstream to design a new interface.
Alternatively, we can disable jit code on arm and mips and give intel
a favor.

 but it needs to be done
 without breaking things for everyone else or knowingly introducing
 suboptimal interfaces that everyone else will suffer from.

Everyone else is a multi-dimentional term. Are you referring to users
of other operating systems or users of arches where sync_icache is noop
or a hypotetical arch where you need to sign code before running it?
(well, it's probably not hypotetical anymore, I vaguely remember reading
about a similar feature few months ago).

As I stated in the earlier email, I can build my rumpkern stuff
conditionally if you split librumpuser into NetBSD and non-NetBSD
versions. I'm going to check how it will work with a private component.

Alex


Re: CVS commit: src/lib/librumpuser

2014-06-17 Thread Valery Ushakov
On Tue, Jun 17, 2014 at 16:20:57 +0200, Joerg Sonnenberger wrote:

 On Tue, Jun 17, 2014 at 06:43:21AM +, Alexander Nasonov wrote:
  Module Name:src
  Committed By:   alnsn
  Date:   Tue Jun 17 06:43:21 UTC 2014
  
  Modified Files:
  src/lib/librumpuser: rumpuser_pth_dummy.c
  
  Log Message:
  For consistency with other files in the same directory
  don't include sys/cdefs.h before __RCSID.
 
 This one is actually wrong, the include should be the first thing in the
 file, followed by __RCSID without conditional.

In that case a local rump header plays that role (either including
cdefs.h or providing the necessary definition).

The conditional is a separate issue and yes it shoud be gc'ed.

-uwe


Re: CVS commit: src/lib/librumpuser

2014-06-17 Thread Alexander Nasonov
Valery Ushakov wrote:
 In that case a local rump header plays that role (either including
 cdefs.h or providing the necessary definition).

Yes, I believe that rumpuser_port.h includes sys/cdefs.h.

 The conditional is a separate issue and yes it shoud be gc'ed.

I agree but I wanted to be consistent with other files in the same
directory.

Alex


Re: CVS commit: src/lib/librumpuser

2014-03-10 Thread Martin Husemann
On Sun, Mar 09, 2014 at 11:01:11PM +, Justin Cormack wrote:
 Module Name:  src
 Committed By: justin
 Date: Sun Mar  9 23:01:11 UTC 2014
 
 Modified Files:
   src/lib/librumpuser: rumpuser_pth.c
 
 Log Message:
 Use __thread rather than pthread_getspecific for rumpuser curlwp.
 This has better performance and curlwp is a performance bottleneck
 in rump kernel code.

You can't do this, it breaks the build on non TLS supporting platforms.

Martin


Re: CVS commit: src/lib/librumpuser

2014-03-10 Thread Justin Cormack
On Mon, Mar 10, 2014 at 8:38 AM, Martin Husemann
mar...@homeworld.netbsd.org wrote:
 On Sun, Mar 09, 2014 at 11:01:11PM +, Justin Cormack wrote:
 Module Name:  src
 Committed By: justin
 Date: Sun Mar  9 23:01:11 UTC 2014

 Modified Files:
   src/lib/librumpuser: rumpuser_pth.c

 Log Message:
 Use __thread rather than pthread_getspecific for rumpuser curlwp.
 This has better performance and curlwp is a performance bottleneck
 in rump kernel code.

 You can't do this, it breaks the build on non TLS supporting platforms.

Argh sorry. Will make it conditional on TLS support. I misread the gcc docs.

Justin


Re: CVS commit: src/lib/librumpuser

2013-09-26 Thread David Laight
On Thu, Sep 26, 2013 at 12:41:51AM +, Mindaugas Rasiukevicius wrote:
 Module Name:  src
 Committed By: rmind
 Date: Thu Sep 26 00:41:51 UTC 2013
 
 Modified Files:
   src/lib/librumpuser: rumpuser_pth.c
 
 Log Message:
 Give RUMP mutex and rwlock their own cache-line.  Also give a separate
 cache-line for the rwlock's reader counter.

You'd be much better off ordering the fields to avoid ping-pong
of data.

IIRC one of the new (big) ppc has 256 byte cache lines.
Cache line aligning data is then getting silly.

David

-- 
David Laight: da...@l8s.co.uk