Re: [PATCH] netbsd32 swapctl, round 4
On Feb 3, 8:04am, m...@netbsd.org (Emmanuel Dreyfus) wrote: -- Subject: Re: [PATCH] netbsd32 swapctl, round 4 | On Mon, Feb 03, 2014 at 03:06:14AM +, Christos Zoulas wrote: | > I thought we decided that it is better to have one sep32 on the stack | > and do copyout in the loop. | | I can do that too, I just need to sort out the size question: ok. | > >+out: | > >+ kmem_free(sep, sizeof(*sep)); | > >+ kmem_free(sep32, sizeof(*sep32)); | > The sizes are wrong. | | How are they wrong? * count? christos
Re: [PATCH] netbsd32 swapctl, round 4
On Mon, Feb 03, 2014 at 10:38:15PM +1100, matthew green wrote: > > > >+kmem_free(sep32, sizeof(*sep32)); > > > The sizes are wrong. > > How are they wrong? > missing * count. ouch. kmem_free() is easy to use wrongly. Yep, no warning. I will now commit with this fixed. -- Emmanuel Dreyfus m...@netbsd.org
re: [PATCH] netbsd32 swapctl, round 4
> On Mon, Feb 03, 2014 at 03:06:14AM +, Christos Zoulas wrote: > > I thought we decided that it is better to have one sep32 on the stack > > and do copyout in the loop. > > I can do that too, I just need to sort out the size question: > > > >+out: > > >+ kmem_free(sep, sizeof(*sep)); > > >+ kmem_free(sep32, sizeof(*sep32)); > > The sizes are wrong. > > How are they wrong? missing * count. ouch. kmem_free() is easy to use wrongly. .mrg.
Re: [PATCH] netbsd32 swapctl, round 4
On Mon, Feb 03, 2014 at 03:06:14AM +, Christos Zoulas wrote: > I thought we decided that it is better to have one sep32 on the stack > and do copyout in the loop. I can do that too, I just need to sort out the size question: > >+out: > >+kmem_free(sep, sizeof(*sep)); > >+kmem_free(sep32, sizeof(*sep32)); > The sizes are wrong. How are they wrong? -- Emmanuel Dreyfus m...@netbsd.org
Re: [PATCH] netbsd32 swapctl, round 4
In article <20140202043534.ga8...@homeworld.netbsd.org>,
Emmanuel Dreyfus wrote:
>Latest revision of the netbsd32 swapctl patch
>
>+ for (i = 0; i < count; i++) {
>+ sep32[i].se_dev = sep[i].se_dev;
>+ sep32[i].se_flags = sep[i].se_flags;
>+ sep32[i].se_nblks = sep[i].se_nblks;
>+ sep32[i].se_inuse = sep[i].se_inuse;
>+ sep32[i].se_priority = sep[i].se_priority;
>+ memcpy(sep32[i].se_path, sep[i].se_path,
>+ sizeof(sep32[i].se_path));
>+ }
>+
>+ error = copyout(sep32, SCARG(uap, arg), sizeof(*sep32) * count);
I thought we decided that it is better to have one sep32 on the stack
and do copyout in the loop.
>+out:
>+ kmem_free(sep, sizeof(*sep));
>+ kmem_free(sep32, sizeof(*sep32));
The sizes are wrong.
christos
Re: [PATCH] netbsd32 swapctl, round 4
On Sun, Feb 02, 2014 at 04:43:49PM +0100, Jean-Yves Migeon wrote: > Even functions like calloc(3) are not required to check for the > overflow themselves when you pass them (number of elements, sizeof > elements). It is required to, otherwise the return value would not be a pointer to an array of the correct size. Joerg
Re: kmem_calloc and overflow checks [was Re: [PATCH] netbsd32 swapctl, round 4]
Jean-Yves Migeon wrote: > We are a bit sliding away from the original swap32 compat code from > Emmanuel though :) No problem, I will commit as is, and you can improve once the issue will be settled (in a few months, hopefully :-) -- Emmanuel Dreyfus http://hcpnet.free.fr/pubz m...@netbsd.org
Re: kmem_calloc and overflow checks [was Re: [PATCH] netbsd32 swapctl, round 4]
Le 02/02/2014 16:56, Taylor R Campbell a écrit : Date: Sun, 02 Feb 2014 16:43:49 +0100 From: Jean-Yves Migeon Even functions like calloc(3) are not required to check for the overflow themselves when you pass them (number of elements, sizeof elements). Overflow checks are rather cumbersome in C... Calloc(3) may not check, but we could define the semantics of kmem_calloc to guarantee an overflow check in order to make it less cumbersome for callers. Yep, right. We are a bit sliding away from the original swap32 compat code from Emmanuel though :) In this case I would leave the overflow checks, as this is not a real performance critical path. It is more a matter of avoiding errors by cargo cult programming if the code gets copy/pasted elsewhere but not adapted correctly. I think that implementing "integer overflow" checks in kernel (and also userland) functions requires to be thought about more thoroughly. It is inconvenient to have APIs with some that do's and some that don'ts. Cheers, -- Jean-Yves Migeon
kmem_calloc and overflow checks [was Re: [PATCH] netbsd32 swapctl, round 4]
Date: Sun, 02 Feb 2014 16:43:49 +0100
From: Jean-Yves Migeon
Even functions like calloc(3) are not required to check for the overflow
themselves when you pass them (number of elements, sizeof elements).
Overflow checks are rather cumbersome in C...
Calloc(3) may not check, but we could define the semantics of
kmem_calloc to guarantee an overflow check in order to make it less
cumbersome for callers.
void *
kmem_calloc(size_t n, size_t size, km_flag_t flags)
{
KASSERT(size != 0);
if (n > (SIZE_MAX / size))
return NULL;
return kmem_zalloc((n * size), flags);
}
Of course, callers of kmem_calloc(..., KM_SLEEP) would have to check
for failure, but that's easier than doing arithmetic to check for
overflow.
Re: [PATCH] netbsd32 swapctl, round 4
Le 02/02/2014 16:04, Taylor R Campbell a écrit : Date: Mon, 03 Feb 2014 00:37:34 +1100 from: matthew green > > + sep = kmem_alloc(sizeof(*sep) * count, KM_SLEEP); > > + sep32 = kmem_alloc(sizeof(*sep32) * count, KM_SLEEP); > > You can overflow "sizeof(*sep) * count", make the kmem_alloc(...) > succeed (the overflow will result in a small size_t if "count" is > properly chosen which is the size kmem_alloc() expects), then corrupt > adjacent kernel memory through the loop when writing into sep32 array. it would require having about 4 million swap devices to trigger this. ... nothing to see here, move right along. :-) Nevertheless, it wouldn't hurt to add if (count > (SIZE_MAX / sizeof(*sep))) fail; if (count > (SIZE_MAX / sizeof(*sep32))) fail; IMHO these kind of checks should always be present at kernel/userland or kernel/network transitions. They are often overly paranoid but help avoids unwanted overflows that get exploited sooner or later. or perhaps to introduce a kmem_calloc which would do this check for us, and that way you could eyeball the code locally to verify its safety without having to reason about the context. Difficult. The overflow happens before kmem_alloc() is called, function has no visibility unless played with dirty tricks with compiler and macros. Even functions like calloc(3) are not required to check for the overflow themselves when you pass them (number of elements, sizeof elements). Overflow checks are rather cumbersome in C... https://www.securecoding.cert.org/confluence/display/seccode/INT32-C.+Ensure+that+operations+on+signed+integers+do+not+result+in+overflow -- Jean-Yves Migeon
Re: [PATCH] netbsd32 swapctl, round 4
Date: Mon, 03 Feb 2014 00:37:34 +1100 from: matthew green > > +sep = kmem_alloc(sizeof(*sep) * count, KM_SLEEP); > > +sep32 = kmem_alloc(sizeof(*sep32) * count, KM_SLEEP); > > You can overflow "sizeof(*sep) * count", make the kmem_alloc(...) > succeed (the overflow will result in a small size_t if "count" is > properly chosen which is the size kmem_alloc() expects), then corrupt > adjacent kernel memory through the loop when writing into sep32 array. it would require having about 4 million swap devices to trigger this. ... nothing to see here, move right along. :-) Nevertheless, it wouldn't hurt to add if (count > (SIZE_MAX / sizeof(*sep))) fail; if (count > (SIZE_MAX / sizeof(*sep32))) fail; or perhaps to introduce a kmem_calloc which would do this check for us, and that way you could eyeball the code locally to verify its safety without having to reason about the context.
Re: [PATCH] netbsd32 swapctl, round 4
Le 02/02/2014 14:37, matthew green a écrit :
+ int count = SCARG(uap, misc);
+ int i, error = 0;
+
+ /* Make sure userland cannot exhaust kernel memory */
+ if ((size_t)count > (size_t)uvmexp.nswapdev)
+ count = uvmexp.nswapdev;
+
+ sep = kmem_alloc(sizeof(*sep) * count, KM_SLEEP);
+ sep32 = kmem_alloc(sizeof(*sep32) * count, KM_SLEEP);
+
+ uvm_swap_stats(SWAP_STATS, sep, count, retval);
+ count = *retval;
+
+ if (count < 1)
+ goto out;
+
+ for (i = 0; i < count; i++) {
+ sep32[i].se_dev = sep[i].se_dev;
+ sep32[i].se_flags = sep[i].se_flags;
+ sep32[i].se_nblks = sep[i].se_nblks;
+ sep32[i].se_inuse = sep[i].se_inuse;
+ sep32[i].se_priority = sep[i].se_priority;
+ memcpy(sep32[i].se_path, sep[i].se_path,
+ sizeof(sep32[i].se_path));
+ }
I don't think that nswapdev can go ridiculously high anyway, however I
think that the bound checks for "count" does prevent kernel memory
exhaustion but not corruption.
You can overflow "sizeof(*sep) * count", make the kmem_alloc(...)
succeed (the overflow will result in a small size_t if "count" is
properly chosen which is the size kmem_alloc() expects), then corrupt
adjacent kernel memory through the loop when writing into sep32 array.
it would require having about 4 million swap devices to trigger this.
... nothing to see here, move right along. :-)
I though that the swapent was way bigger with the PATH_MAX component,
but it is about 4k, so... that makes for a whole lot of swap devices
indeed :)
--
Jean-Yves Migeon
re: [PATCH] netbsd32 swapctl, round 4
> > + int count = SCARG(uap, misc);
> > + int i, error = 0;
> > +
> > + /* Make sure userland cannot exhaust kernel memory */
> > + if ((size_t)count > (size_t)uvmexp.nswapdev)
> > + count = uvmexp.nswapdev;
> > +
> > + sep = kmem_alloc(sizeof(*sep) * count, KM_SLEEP);
> > + sep32 = kmem_alloc(sizeof(*sep32) * count, KM_SLEEP);
> > +
> > + uvm_swap_stats(SWAP_STATS, sep, count, retval);
> > + count = *retval;
> > +
> > + if (count < 1)
> > + goto out;
> > +
> > + for (i = 0; i < count; i++) {
> > + sep32[i].se_dev = sep[i].se_dev;
> > + sep32[i].se_flags = sep[i].se_flags;
> > + sep32[i].se_nblks = sep[i].se_nblks;
> > + sep32[i].se_inuse = sep[i].se_inuse;
> > + sep32[i].se_priority = sep[i].se_priority;
> > + memcpy(sep32[i].se_path, sep[i].se_path,
> > + sizeof(sep32[i].se_path));
> > + }
>
> I don't think that nswapdev can go ridiculously high anyway, however I
> think that the bound checks for "count" does prevent kernel memory
> exhaustion but not corruption.
>
> You can overflow "sizeof(*sep) * count", make the kmem_alloc(...)
> succeed (the overflow will result in a small size_t if "count" is
> properly chosen which is the size kmem_alloc() expects), then corrupt
> adjacent kernel memory through the loop when writing into sep32 array.
it would require having about 4 million swap devices to trigger this.
... nothing to see here, move right along. :-)
.mrg.
Re: [PATCH] netbsd32 swapctl, round 4
Le 02/02/2014 05:35, Emmanuel Dreyfus a écrit :
[snip]
+ int count = SCARG(uap, misc);
+ int i, error = 0;
+
+ /* Make sure userland cannot exhaust kernel memory */
+ if ((size_t)count > (size_t)uvmexp.nswapdev)
+ count = uvmexp.nswapdev;
+
+ sep = kmem_alloc(sizeof(*sep) * count, KM_SLEEP);
+ sep32 = kmem_alloc(sizeof(*sep32) * count, KM_SLEEP);
+
+ uvm_swap_stats(SWAP_STATS, sep, count, retval);
+ count = *retval;
+
+ if (count < 1)
+ goto out;
+
+ for (i = 0; i < count; i++) {
+ sep32[i].se_dev = sep[i].se_dev;
+ sep32[i].se_flags = sep[i].se_flags;
+ sep32[i].se_nblks = sep[i].se_nblks;
+ sep32[i].se_inuse = sep[i].se_inuse;
+ sep32[i].se_priority = sep[i].se_priority;
+ memcpy(sep32[i].se_path, sep[i].se_path,
+ sizeof(sep32[i].se_path));
+ }
I don't think that nswapdev can go ridiculously high anyway, however I
think that the bound checks for "count" does prevent kernel memory
exhaustion but not corruption.
You can overflow "sizeof(*sep) * count", make the kmem_alloc(...)
succeed (the overflow will result in a small size_t if "count" is
properly chosen which is the size kmem_alloc() expects), then corrupt
adjacent kernel memory through the loop when writing into sep32 array.
Cheers,
--
Jean-Yves Migeon
re: [PATCH] netbsd32 swapctl, round 4
> Latest revision of the netbsd32 swapctl patch this looks good now. thanks for dealing with this properly! .mrg.
