Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Pedro Giffuni]

On 01/27/18 11:14, Warner Losh wrote:



On Jan 27, 2018 8:17 AM, "Pedro Giffuni" > wrote:




On 01/26/18 06:36, Bruce Evans wrote:

On Thu, 25 Jan 2018, Pedro Giffuni wrote:

On 25/01/2018 14:24, Bruce Evans wrote:

...
This code only works because (if?) nfs is the only
caller and nfs never
passes insane values.


I am starting to think that we should simply match
uio_resid and set it to ssize_t.
Returning the value to int is certainly not the solution.


Of course using the correct type (int) is part of the solution.

uio_must be checked before it is used for cookies, and after
checking it, it
is small so it fits easily in an int.  It must also checked to
be nonnegative,
so that it doesn't suffer unsigned poisoning when it is
promoted, so it would
also fit in a u_int, but using u_int to store it is silly as
using 1U instead
of 1 for a count of 1.

The bounds checking is something like:

if (ap->uio_resid < 0)
    ap->uio_resid = 0;
if (ap->a_ncookies != NULL) {
    if (ap->uio_resid >= 64 * 1024)
    ap->uio_resid = 64 * 1024;
    ncookies = ap->uio_resid;
}

This checks for negative values for all cases and converts to
0 (EOF) to
preserve historical behaviour for the syscall case and to
avoid overflow
for the cookies case (in case the caller is buggy). The
correct handling
is to return EINVAL, but EOF is good enough.

In the syscall case, uio_resid can be up to SSIZE_MAX, so
don't check it
or corrupt it by assigning it to an int or u_int.

Limit uio_resid from above only in the cookies case.  The
final limit should
be about 128K (whatever nfs uses) or maybe 1M. Don't return
EINVAL above
the limit, since nfs probably wouldn't know how to handle that
(by retrying
with a smaller size).  Test its handling of short counts
instead. It is
expected than nfs asks for 128K and we supply at most 64K. 
The supply is
always reduced at EOF.  Hopefully nfs doesn't treat the short
count as EOF.
It should retry until we supply 0.

Hmm ...

We have never checked the upper bound there, which doesn't mean it
was right.
I found MAXPHYS, which seems a more reasonable limit used in the
kernel for uio_resid.

I am checking the patch compiles and doesn't give surprises.


MAXPHYS is almost the right thing to check. There's per device limits 
for normal I/O, but that doesn't seem to be a strict limit for readdir.




OK... new patch, this time again trying to sanitize only ncookies (which 
can be int or u_int, doesn't matter to me).


Pedro.

Index: sys/fs/ext2fs/ext2_lookup.c
===
--- sys/fs/ext2fs/ext2_lookup.c	(revision 328478)
+++ sys/fs/ext2fs/ext2_lookup.c	(working copy)
@@ -145,7 +145,7 @@
 	off_t offset, startoffset;
 	size_t readcnt, skipcnt;
 	ssize_t startresid;
-	u_int ncookies;
+	int ncookies;
 	int DIRBLKSIZ = VTOI(ap->a_vp)->i_e2fs->e2fs_bsize;
 	int error;
 
@@ -152,7 +152,11 @@
 	if (uio->uio_offset < 0)
 		return (EINVAL);
 	ip = VTOI(vp);
+	if (uio->uio_resid < 0)
+		uio->uio_resid = 0;
 	if (ap->a_ncookies != NULL) {
+		if (uio->uio_resid > MAXPHYS)
+			uio->uio_resid = MAXPHYS;
 		ncookies = uio->uio_resid;
 		if (uio->uio_offset >= ip->i_size)
 			ncookies = 0;
Index: sys/ufs/ufs/ufs_vnops.c
===
--- sys/ufs/ufs/ufs_vnops.c	(revision 328478)
+++ sys/ufs/ufs/ufs_vnops.c	(working copy)
@@ -2170,7 +2170,7 @@
 	off_t offset, startoffset;
 	size_t readcnt, skipcnt;
 	ssize_t startresid;
-	u_int ncookies;
+	int ncookies;
 	int error;
 
 	if (uio->uio_offset < 0)
@@ -2178,7 +2178,11 @@
 	ip = VTOI(vp);
 	if (ip->i_effnlink == 0)
 		return (0);
+	if (uio->uio_resid < 0)
+		uio->uio_resid = 0;
 	if (ap->a_ncookies != NULL) {
+		if (uio->uio_resid > MAXPHYS)
+			uio->uio_resid = MAXPHYS;
 		ncookies = uio->uio_resid;
 		if (uio->uio_offset >= ip->i_size)
 			ncookies = 0;
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Warner Losh]

On Jan 27, 2018 8:17 AM, "Pedro Giffuni"  wrote:



On 01/26/18 06:36, Bruce Evans wrote:

> On Thu, 25 Jan 2018, Pedro Giffuni wrote:
>
> On 25/01/2018 14:24, Bruce Evans wrote:
>>
>>> ...
>>> This code only works because (if?) nfs is the only caller and nfs never
>>> passes insane values.
>>>
>>>
>> I am starting to think that we should simply match uio_resid and set it
>> to ssize_t.
>> Returning the value to int is certainly not the solution.
>>
>
> Of course using the correct type (int) is part of the solution.
>
> uio_must be checked before it is used for cookies, and after checking it,
> it
> is small so it fits easily in an int.  It must also checked to be
> nonnegative,
> so that it doesn't suffer unsigned poisoning when it is promoted, so it
> would
> also fit in a u_int, but using u_int to store it is silly as using 1U
> instead
> of 1 for a count of 1.
>
> The bounds checking is something like:
>
> if (ap->uio_resid < 0)
> ap->uio_resid = 0;
> if (ap->a_ncookies != NULL) {
> if (ap->uio_resid >= 64 * 1024)
> ap->uio_resid = 64 * 1024;
> ncookies = ap->uio_resid;
> }
>
> This checks for negative values for all cases and converts to 0 (EOF) to
> preserve historical behaviour for the syscall case and to avoid overflow
> for the cookies case (in case the caller is buggy).  The correct handling
> is to return EINVAL, but EOF is good enough.
>
> In the syscall case, uio_resid can be up to SSIZE_MAX, so don't check it
> or corrupt it by assigning it to an int or u_int.
>
> Limit uio_resid from above only in the cookies case.  The final limit
> should
> be about 128K (whatever nfs uses) or maybe 1M.  Don't return EINVAL above
> the limit, since nfs probably wouldn't know how to handle that (by retrying
> with a smaller size).  Test its handling of short counts instead. It is
> expected than nfs asks for 128K and we supply at most 64K.  The supply is
> always reduced at EOF.  Hopefully nfs doesn't treat the short count as EOF.
> It should retry until we supply 0.
>
> Hmm ...

We have never checked the upper bound there, which doesn't mean it was
right.
I found MAXPHYS, which seems a more reasonable limit used in the kernel for
uio_resid.

I am checking the patch compiles and doesn't give surprises.


MAXPHYS is almost the right thing to check. There's per device limits for
normal I/O, but that doesn't seem to be a strict limit for readdir.

Warner


Pedro.


After limiting uio_resid, assign it to the int ncookies.
>
> This doesn't fix the abuse of the ncookies counter to hold the size of the
> cookies array in bytes for this and the next couple of statements.
>
> Normally the bounds checking should be at the top level, with at most
> KASSERT()s at lower levels, but here the levels are mixed, and it isn't
> clear if kernel callers have already checked, and it doesn't cost much to
> do much the same checking for the kernel callers as for the syscall callers.
>
> Perhaps the 128K limit is good for all cases (this depends on callers not
> having buggy short count handling).  Directories of this size are very
> rare (don't forget to create very large ones when you test this). Doing
> anything with directories of this size tends to be slow anyway, and the
> slowness has nothing to do with reading only 128K instead of SSIZE_MAX
> bytes at a time.
>
> readdir() in FreeBSD seems to use a read size of only PAGE_SIZE, except
> in the unionfs case it seems to try to read the whole direction. It
> malloc()s the buffer in both cases.  Blindy malloc()ing or mmap()ing
> a buffer large enough for a whole file or directory is no good, since
> in theory even directory sizes can be much larger than memory.
>
> Bruce
>
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Pedro Giffuni]

On 01/26/18 06:36, Bruce Evans wrote:

On Thu, 25 Jan 2018, Pedro Giffuni wrote:


On 25/01/2018 14:24, Bruce Evans wrote:

...
This code only works because (if?) nfs is the only caller and nfs never
passes insane values.



I am starting to think that we should simply match uio_resid and set 
it to ssize_t.

Returning the value to int is certainly not the solution.


Of course using the correct type (int) is part of the solution.

uio_must be checked before it is used for cookies, and after checking 
it, it
is small so it fits easily in an int.  It must also checked to be 
nonnegative,
so that it doesn't suffer unsigned poisoning when it is promoted, so 
it would
also fit in a u_int, but using u_int to store it is silly as using 1U 
instead

of 1 for a count of 1.

The bounds checking is something like:

if (ap->uio_resid < 0)
    ap->uio_resid = 0;
if (ap->a_ncookies != NULL) {
    if (ap->uio_resid >= 64 * 1024)
    ap->uio_resid = 64 * 1024;
    ncookies = ap->uio_resid;
}

This checks for negative values for all cases and converts to 0 (EOF) to
preserve historical behaviour for the syscall case and to avoid overflow
for the cookies case (in case the caller is buggy).  The correct handling
is to return EINVAL, but EOF is good enough.

In the syscall case, uio_resid can be up to SSIZE_MAX, so don't check it
or corrupt it by assigning it to an int or u_int.

Limit uio_resid from above only in the cookies case.  The final limit 
should

be about 128K (whatever nfs uses) or maybe 1M.  Don't return EINVAL above
the limit, since nfs probably wouldn't know how to handle that (by 
retrying

with a smaller size).  Test its handling of short counts instead. It is
expected than nfs asks for 128K and we supply at most 64K.  The supply is
always reduced at EOF.  Hopefully nfs doesn't treat the short count as 
EOF.

It should retry until we supply 0.


Hmm ...

We have never checked the upper bound there, which doesn't mean it was 
right.
I found MAXPHYS, which seems a more reasonable limit used in the kernel 
for uio_resid.


I am checking the patch compiles and doesn't give surprises.

Pedro.


After limiting uio_resid, assign it to the int ncookies.

This doesn't fix the abuse of the ncookies counter to hold the size of 
the

cookies array in bytes for this and the next couple of statements.

Normally the bounds checking should be at the top level, with at most
KASSERT()s at lower levels, but here the levels are mixed, and it isn't
clear if kernel callers have already checked, and it doesn't cost much 
to do much the same checking for the kernel callers as for the syscall 
callers.


Perhaps the 128K limit is good for all cases (this depends on callers not
having buggy short count handling).  Directories of this size are very
rare (don't forget to create very large ones when you test this). Doing
anything with directories of this size tends to be slow anyway, and the
slowness has nothing to do with reading only 128K instead of SSIZE_MAX
bytes at a time.

readdir() in FreeBSD seems to use a read size of only PAGE_SIZE, except
in the unionfs case it seems to try to read the whole direction. It
malloc()s the buffer in both cases.  Blindy malloc()ing or mmap()ing
a buffer large enough for a whole file or directory is no good, since
in theory even directory sizes can be much larger than memory.

Bruce



___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Bruce Evans]

On Fri, 26 Jan 2018, Pedro Giffuni wrote:


On 01/26/18 06:36, Bruce Evans wrote:

On Thu, 25 Jan 2018, Pedro Giffuni wrote:


On 25/01/2018 14:24, Bruce Evans wrote:

...
This code only works because (if?) nfs is the only caller and nfs never
passes insane values.



I am starting to think that we should simply match uio_resid and set it to 
ssize_t.

Returning the value to int is certainly not the solution.


Of course using the correct type (int) is part of the solution.


int *was* the correct type, now it doesn't cover all the range.


int is the correct type.  The range is small after range checking.

u_int is an incorrect type.  It can only hold SSIZE_MAX on __LP32_ arches,
and that only after half-baked range checking for the lower limit only.

ssize_t is an incorrect type.  It can hold SSIZE_MAX on all arches, but
that doesn't prevent various overflow bugs or requests to malloc()
SSIZE_MAX (plus a little more for rounding up) bytes unless there is
some range checking.

Our problem is not really uio_*, our problem is ncookies and we only test 
that it is >0.


Our problem is lack of understanding of lectures on C types and range checking.

I think the attached patch, still keeping the unsigned ncookies, is 
sufficient.


X Index: sys/fs/ext2fs/ext2_lookup.c
X ===
X --- sys/fs/ext2fs/ext2_lookup.c   (revision 328443)
X +++ sys/fs/ext2fs/ext2_lookup.c   (working copy)
X @@ -153,7 +153,10 @@
X   return (EINVAL);
X   ip = VTOI(vp);
X   if (ap->a_ncookies != NULL) {
X - ncookies = uio->uio_resid;
X + if (uio->uio_resid < 0)
X + ncookies = 0;
X + else
X + ncookies = uio->uio_resid;

This only avoids overflow for negative values.  It doesn't limit uio_resid
from above, so overflow on this assignment can occur on __LP64__ arches.
Even on __LP32__ systems, SSIZE_MAX = 2G is too large to work.  My patch
limits uio_resid to avoid this and many other bugs.

When overflow occurs, this gives the same bugs as before, but they are
larger than I noticed before.  E.g., uio->uio_resid == 1ULL << 32, then
the overflow is to 0.  There is no problem allocating a cookies array
with 0 elements, but since you didn't adjust uio_resid it is inconsistent
with ncookies.  Without INVARIANTS, buffer overrun occurs when the first
dirent is read and cookies[0] is modified to point to the dirent; then
ncookies was decremented from 0 to -1, but after you broke its type it
is decremented from 0 to UINT_MAX.  With INVARIANTS, the bug is detected
by a KASSERT() that ncookies > 0.

X   if (uio->uio_offset >= ip->i_size)
X   ncookies = 0;
X   else if (ip->i_size - uio->uio_offset < ncookies)

When uio_resid is huge, that isn't usually a problem unless assigning it
to ncookies gives a small value.  Otherwise, ncookies is at least large
here.  Then it is usually larger than the residual file size, so it gets
replaced by the residual file size.  This is only too large to fit in
memory if the residual file size is large.


The bounds checking is something like:



[... context lost to corruption of spaces to binary (UTF-8))



This checks for negative values for all cases and converts to 0 (EOF) to
preserve historical behaviour for the syscall case and to avoid overflow
for the cookies case (in case the caller is buggy).?? The correct handling
is to return EINVAL, but EOF is good enough.

This also touches uio_resid which is probably not good as it is used later 
on.

Our job is not to "fix" the caller but only to apply a reasonable behavior.


This indeed too fragile.  I had throught that syscalls adjusted uio_resid
at the end (so could adjust it at the beginning too).  They actually just
calculate nbytes = initial_uio_resid - final_uio_resid.

But if you don't adjust uio_resid, then you get buffer overruns when there
are more dirents than cookies.  dirents are not malloc()ed, so there is no
limit on them except uio_resid, the size that can be malloc()ed for cookies,
and the residual file size.


In the syscall case, uio_resid can be up to SSIZE_MAX, so don't check it
or corrupt it by assigning it to an int or u_int.


The minimal type conversion does not really involve corruption: ncookies is 
local

and the caller will not perceive any change.


The value is corrupted by blind assignmemt to an unrelated type.

Bruce___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Pedro Giffuni]

On 01/26/18 06:36, Bruce Evans wrote:

On Thu, 25 Jan 2018, Pedro Giffuni wrote:


On 25/01/2018 14:24, Bruce Evans wrote:

...
This code only works because (if?) nfs is the only caller and nfs never
passes insane values.



I am starting to think that we should simply match uio_resid and set 
it to ssize_t.

Returning the value to int is certainly not the solution.


Of course using the correct type (int) is part of the solution.


int *was* the correct type, now it doesn't cover all the range.

uio_must be checked before it is used for cookies, and after checking 
it, it
is small so it fits easily in an int.  It must also checked to be 
nonnegative,


Our problem is not really uio_*, our problem is ncookies and we only 
test that it is >0.
I think the attached patch, still keeping the unsigned ncookies, is 
sufficient.




so that it doesn't suffer unsigned poisoning when it is promoted, so 
it would
also fit in a u_int, but using u_int to store it is silly as using 1U 
instead

of 1 for a count of 1.

The bounds checking is something like:

if (ap->uio_resid < 0)
    ap->uio_resid = 0;
if (ap->a_ncookies != NULL) {
    if (ap->uio_resid >= 64 * 1024)
    ap->uio_resid = 64 * 1024;
    ncookies = ap->uio_resid;
}

This checks for negative values for all cases and converts to 0 (EOF) to
preserve historical behaviour for the syscall case and to avoid overflow
for the cookies case (in case the caller is buggy).  The correct handling
is to return EINVAL, but EOF is good enough.

This also touches uio_resid which is probably not good as it is used 
later on.

Our job is not to "fix" the caller but only to apply a reasonable behavior.

I also don't like the magic numbers here.


In the syscall case, uio_resid can be up to SSIZE_MAX, so don't check it
or corrupt it by assigning it to an int or u_int.



The minimal type conversion does not really involve corruption: ncookies 
is local

and the caller will not perceive any change.

Pedro.

Limit uio_resid from above only in the cookies case.  The final limit 
should

be about 128K (whatever nfs uses) or maybe 1M.  Don't return EINVAL above
the limit, since nfs probably wouldn't know how to handle that (by 
retrying

with a smaller size).  Test its handling of short counts instead. It is
expected than nfs asks for 128K and we supply at most 64K.  The supply is
always reduced at EOF.  Hopefully nfs doesn't treat the short count as 
EOF.

It should retry until we supply 0.

After limiting uio_resid, assign it to the int ncookies.

This doesn't fix the abuse of the ncookies counter to hold the size of 
the

cookies array in bytes for this and the next couple of statements.

Normally the bounds checking should be at the top level, with at most
KASSERT()s at lower levels, but here the levels are mixed, and it isn't
clear if kernel callers have already checked, and it doesn't cost much 
to do much the same checking for the kernel callers as for the syscall 
callers.


Perhaps the 128K limit is good for all cases (this depends on callers not
having buggy short count handling).  Directories of this size are very
rare (don't forget to create very large ones when you test this). Doing
anything with directories of this size tends to be slow anyway, and the
slowness has nothing to do with reading only 128K instead of SSIZE_MAX
bytes at a time.

readdir() in FreeBSD seems to use a read size of only PAGE_SIZE, except
in the unionfs case it seems to try to read the whole direction. It
malloc()s the buffer in both cases.  Blindy malloc()ing or mmap()ing
a buffer large enough for a whole file or directory is no good, since
in theory even directory sizes can be much larger than memory.

Bruce


Index: sys/fs/ext2fs/ext2_lookup.c
===
--- sys/fs/ext2fs/ext2_lookup.c	(revision 328443)
+++ sys/fs/ext2fs/ext2_lookup.c	(working copy)
@@ -153,7 +153,10 @@
 		return (EINVAL);
 	ip = VTOI(vp);
 	if (ap->a_ncookies != NULL) {
-		ncookies = uio->uio_resid;
+		if (uio->uio_resid < 0)
+			ncookies = 0;
+		else
+			ncookies = uio->uio_resid;
 		if (uio->uio_offset >= ip->i_size)
 			ncookies = 0;
 		else if (ip->i_size - uio->uio_offset < ncookies)
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Bruce Evans]

On Thu, 25 Jan 2018, Pedro Giffuni wrote:


On 25/01/2018 14:24, Bruce Evans wrote:

...
This code only works because (if?) nfs is the only caller and nfs never
passes insane values.



I am starting to think that we should simply match uio_resid and set it to 
ssize_t.

Returning the value to int is certainly not the solution.


Of course using the correct type (int) is part of the solution.

uio_must be checked before it is used for cookies, and after checking it, it
is small so it fits easily in an int.  It must also checked to be nonnegative,
so that it doesn't suffer unsigned poisoning when it is promoted, so it would
also fit in a u_int, but using u_int to store it is silly as using 1U instead
of 1 for a count of 1.

The bounds checking is something like:

if (ap->uio_resid < 0)
ap->uio_resid = 0;
if (ap->a_ncookies != NULL) {
if (ap->uio_resid >= 64 * 1024)
ap->uio_resid = 64 * 1024;
ncookies = ap->uio_resid;
}

This checks for negative values for all cases and converts to 0 (EOF) to
preserve historical behaviour for the syscall case and to avoid overflow
for the cookies case (in case the caller is buggy).  The correct handling
is to return EINVAL, but EOF is good enough.

In the syscall case, uio_resid can be up to SSIZE_MAX, so don't check it
or corrupt it by assigning it to an int or u_int.

Limit uio_resid from above only in the cookies case.  The final limit should
be about 128K (whatever nfs uses) or maybe 1M.  Don't return EINVAL above
the limit, since nfs probably wouldn't know how to handle that (by retrying
with a smaller size).  Test its handling of short counts instead.  It is
expected than nfs asks for 128K and we supply at most 64K.  The supply is
always reduced at EOF.  Hopefully nfs doesn't treat the short count as EOF.
It should retry until we supply 0.

After limiting uio_resid, assign it to the int ncookies.

This doesn't fix the abuse of the ncookies counter to hold the size of the
cookies array in bytes for this and the next couple of statements.

Normally the bounds checking should be at the top level, with at most
KASSERT()s at lower levels, but here the levels are mixed, and it isn't
clear if kernel callers have already checked, and it doesn't cost much to 
do much the same checking for the kernel callers as for the syscall callers.


Perhaps the 128K limit is good for all cases (this depends on callers not
having buggy short count handling).  Directories of this size are very
rare (don't forget to create very large ones when you test this).  Doing
anything with directories of this size tends to be slow anyway, and the
slowness has nothing to do with reading only 128K instead of SSIZE_MAX
bytes at a time.

readdir() in FreeBSD seems to use a read size of only PAGE_SIZE, except
in the unionfs case it seems to try to read the whole direction.  It
malloc()s the buffer in both cases.  Blindy malloc()ing or mmap()ing
a buffer large enough for a whole file or directory is no good, since
in theory even directory sizes can be much larger than memory.

Bruce
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Pedro Giffuni]

On 25/01/2018 14:24, Bruce Evans wrote:

On Thu, 25 Jan 2018, Pedro Giffuni wrote:

This is almost unreadable due to hard-coded UTF-8 (mostly for tabs 
corrupted

to spaces) even in previously-literally quoted C code.


Mailer agents ... they all suck :(

On 01/25/18 11:28, Bruce Evans wrote:

On Wed, 24 Jan 2018, Pedro F. Giffuni wrote:


[... Most unreadable lines deleted]


X ncookies = uio->uio_resid;

This has a more serious type error and consequent overflow bugs. 
uio_resid
used to have type int, and cookies had to have type int to match 
that, else

there overflow occurs before the bounds checks.  Now uio_resid has type
ssize_t, which is excessively large on 64-bit arches (64 bits then), 
so the
assignment overflowed when ncookies had type int and uio_resid > 
INT_MAX.
Now it overflows differently when uio_resid > UINT_MAX, and unsign 
extension

causes overflow when uio_resid < 0.  There might be a sanity check on
uio_resid at higher levels, but I can only find a check related to 
EOF in

vfs_read_dirent().

I will argue that none of the code in this function is prepared for 
the eventually of

uio->uio_resid < 0


All of it except the cookies code has to be prepared for that, and seems
to handle it OK, since this userland can set uio_resid.  The other code
is not broken by either the ssize_t expansion or the unsigned bugs, since
it mostly doesn't truncate uio_resid by assigning it to a variable of the
wrong type (it uses uio->uio_resid in-place, except for copying its 
initial
value to startresid, and startresid is not missing the ssize_t 
expansion).

It mostly does comparisons of the form (uio->uio_resid > 0), where it is
0 in uio_resid means EOF, negative is treated as EOF, and strictly 
positive

means more to do.

There is a clear up-front check that uio_offset >= 0 (return EINVAL if
uio_offset < 0).  This is not needed for the trusted nfs caller, but is
needed for syscalls and is done for both.


In that case we would have a rather spectacular failure in malloc.
Unsigning ncookies is a theoretical, although likely impractical, 
improvement here.


No, it increases the bug slightly.  E.g., if uio_resid is -1, ncookies
was -1 / (offsetof(...) + 4) + 1 = 0 + 1 after rounding.  This might even
work (1 cookie at a time, just like if the caller asked for that).  Now
ncookies is -1U / (offsetof(...) + 4) + 1 = a large value. However, if
uio_resid was slightly more negative than -2 * (offsetof(...) + 4), then
ncookies was -1 and in the multiplication this overflows to -1U = a large
value and the result is much the same as for earlier overflow on 
assignment

to u_int ncookies.

This code only works because (if?) nfs is the only caller and nfs never
passes insane values.



I am starting to think that we should simply match uio_resid and set it 
to ssize_t.

Returning the value to int is certainly not the solution.

It is not clear to me that using int or u_int makes a difference 
given it is a local variable
and in this scope the signedness of the variable is basically 
irrelevant.


It is clear to me that overflow bugs occur with both if untrusted 
callers are

allowed.

From a logical point of view .. we can't really have a negative 
number of cookies.


Malicious and buggy callers do illogical things to get through holes in
your logic.

It is also illogical to have a zero number of cookies, but ncookies can
be 0 in various ways.  First, ncookies is 0 in the EOF case (and cookies
are requested).  We depend on 0 not being an invalid size for malloc()
so that we malloc() nothing and later do more nothings in the main loop.
This is a standard use for 0.  If you don't like negative numbers, then
you also shouldn't like 0.  Both exist to make some calculations easier.
Later, ncookies is abused as a residual count, so it becomes 0 at the 
end.

This is another standard use for 0.

Bruce


___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Bruce Evans]

On Thu, 25 Jan 2018, Pedro Giffuni wrote:

This is almost unreadable due to hard-coded UTF-8 (mostly for tabs corrupted
to spaces) even in previously-literally quoted C code.


On 01/25/18 11:28, Bruce Evans wrote:

On Wed, 24 Jan 2018, Pedro F. Giffuni wrote:


[... Most unreadable lines deleted]


X ncookies = uio->uio_resid;

This has a more serious type error and consequent overflow bugs. uio_resid
used to have type int, and cookies had to have type int to match that, else
there overflow occurs before the bounds checks.?? Now uio_resid has type
ssize_t, which is excessively large on 64-bit arches (64 bits then), so the
assignment overflowed when ncookies had type int and uio_resid > INT_MAX.
Now it overflows differently when uio_resid > UINT_MAX, and unsign 
extension

causes overflow when uio_resid < 0.?? There might be a sanity check on
uio_resid at higher levels, but I can only find a check related to EOF in
vfs_read_dirent().

I will argue that none of the code in this function is prepared for the 
eventually of

uio->uio_resid < 0


All of it except the cookies code has to be prepared for that, and seems
to handle it OK, since this userland can set uio_resid.  The other code
is not broken by either the ssize_t expansion or the unsigned bugs, since
it mostly doesn't truncate uio_resid by assigning it to a variable of the
wrong type (it uses uio->uio_resid in-place, except for copying its initial
value to startresid, and startresid is not missing the ssize_t expansion).
It mostly does comparisons of the form (uio->uio_resid > 0), where it is
0 in uio_resid means EOF, negative is treated as EOF, and strictly positive
means more to do.

There is a clear up-front check that uio_offset >= 0 (return EINVAL if
uio_offset < 0).  This is not needed for the trusted nfs caller, but is
needed for syscalls and is done for both.


In that case we would have a rather spectacular failure in malloc.
Unsigning ncookies is a theoretical, although likely impractical, improvement 
here.


No, it increases the bug slightly.  E.g., if uio_resid is -1, ncookies
was -1 / (offsetof(...) + 4) + 1 = 0 + 1 after rounding.  This might even
work (1 cookie at a time, just like if the caller asked for that).  Now
ncookies is -1U / (offsetof(...) + 4) + 1 = a large value.  However, if
uio_resid was slightly more negative than -2 * (offsetof(...) + 4), then
ncookies was -1 and in the multiplication this overflows to -1U = a large
value and the result is much the same as for earlier overflow on assignment
to u_int ncookies.

This code only works because (if?) nfs is the only caller and nfs never
passes insane values.

It is not clear to me that using int or u_int makes a difference given it is 
a local variable

and in this scope the signedness of the variable is basically irrelevant.


It is clear to me that overflow bugs occur with both if untrusted callers are
allowed.

From a logical point of view .. we can't really have a negative number of 
cookies.


Malicious and buggy callers do illogical things to get through holes in
your logic.

It is also illogical to have a zero number of cookies, but ncookies can
be 0 in various ways.  First, ncookies is 0 in the EOF case (and cookies
are requested).  We depend on 0 not being an invalid size for malloc()
so that we malloc() nothing and later do more nothings in the main loop.
This is a standard use for 0.  If you don't like negative numbers, then
you also shouldn't like 0.  Both exist to make some calculations easier.
Later, ncookies is abused as a residual count, so it becomes 0 at the end.
This is another standard use for 0.

Bruce___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Pedro Giffuni]

On 01/25/18 11:28, Bruce Evans wrote:

On Wed, 24 Jan 2018, Pedro F. Giffuni wrote:


Log:
 ext2fs|ufs:Unsign some values related to allocation.

 When allocating memory through malloc(9), we always expect the 
amount of
 memory requested to be unsigned as a negative value would either 
stand for

 an error or an overflow.
 Unsign some values, found when considering the use of 
mallocarray(9), to

 avoid unnecessary casting. Also consider that indexes should be of
 at least the same size/type as the upper limit they pretend to index.


This might not break much, but it adds many more type errors and bogus
(implicit) casts than it fixes.  It actually changes the brokenness of 
the

first variable touched:


Modified: head/sys/fs/ext2fs/ext2_lookup.c
== 

--- head/sys/fs/ext2fs/ext2_lookup.c    Wed Jan 24 17:52:06 2018    
(r328345)
+++ head/sys/fs/ext2fs/ext2_lookup.c    Wed Jan 24 17:58:48 2018    
(r328346)

@@ -145,9 +145,9 @@ ext2_readdir(struct vop_readdir_args *ap)
off_t offset, startoffset;
size_t readcnt, skipcnt;
ssize_t startresid;
-    int ncookies;
int DIRBLKSIZ = VTOI(ap->a_vp)->i_e2fs->e2fs_bsize;
int error;
+    u_int ncookies;


The first bug is only a style bug (unsorting the u_int after the ints 
even

when its variable was accidentally already before the ints.


Yup, my error.



if (uio->uio_offset < 0)
    return (EINVAL);


That is the only change in this file.  No comment on other changes.

ncookies is mostly used in contexts other than for multiplying by it 
before
calling malloc(), and its type is now inconsistent with most other 
places.


The other places are:

X ncookies = uio->uio_resid;

This has a more serious type error and consequent overflow bugs. 
uio_resid
used to have type int, and cookies had to have type int to match that, 
else

there overflow occurs before the bounds checks.  Now uio_resid has type
ssize_t, which is excessively large on 64-bit arches (64 bits then), 
so the

assignment overflowed when ncookies had type int and uio_resid > INT_MAX.
Now it overflows differently when uio_resid > UINT_MAX, and unsign 
extension

causes overflow when uio_resid < 0.  There might be a sanity check on
uio_resid at higher levels, but I can only find a check related to EOF in
vfs_read_dirent().

I will argue that none of the code in this function is prepared for the 
eventually of

uio->uio_resid < 0

In that case we would have a rather spectacular failure in malloc.
Unsigning ncookies is a theoretical, although likely impractical, 
improvement here.


It is not clear to me that using int or u_int makes a difference given 
it is a local variable

and in this scope the signedness of the variable is basically irrelevant.

From a logical point of view .. we can't really have a negative number 
of cookies.


Next, we do some bounds checking which seems to be correct modulo 
previous
overflows, and show the care needed for unsigned variables (i_size is 
unsigned

and must be compared with uio_offset which is signed).

Next, we assign ncookies, to ap_ncookies which still has the correct type
(plain signed int).  If ncookies were actually large enough to need a 
u_int,

or worse a 64-bit ssize_t, then this would overflow.

Later, we KASSERT() that ncookies > 0.  This might cause a compiler 
warning
"unsigned comparison with 0" now that ncookies is unsigned.  We count 
down

ncookies, but the loop termination condition is complicated and we don't
get any benefits from the possible micro-optimization of using 
ncookies as

a loop counter that counts down to 0.

To fix the problem that mallocarray() wanted a size_t arg, ncookies could
be cast to size_t, but that would be silly.  The prototype does the same
cast automatically even for cases with sign mismatches.  Now malloc()
wants a type of size_t (after further breakage to change malloc()s arg
type).  Many conversions are still involved, and casting would at most 
limit

compiler warnings:  the code is now:

X malloc(ncookies * sizeof(*cookies), ...)

First, ncookies and sizeof(...) are promoted to a common type. When 
ncookies
was int, usually it was promoted to size_t and sizeof(...) was not 
promoted.
But on exotic arches with size_t smaller than int, sizeof(...) is 
promoted to

int and ncookies is not promoted.

Next, the type of the result of the multiplication is the common type.

Finally, the prototype used to convert to u_long, but now converts to 
size_t.

When the common type is size_t, then the conversion is now null. When the
common type was int, the conversion was promotion to u_long, but it is 
now

demotion to size_t.

All this only obviously works in practice because all the variables and
the product are small, so they are smaller than all of INT_MAX, SIZE_MAX
and ULONG_MAX on all supported and unsupported arches.  However, it is
hard to write bounds checks that obviously handle all cases, except
by using 

Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Bruce Evans]

On Wed, 24 Jan 2018, Pedro F. Giffuni wrote:


Log:
 ext2fs|ufs:Unsign some values related to allocation.

 When allocating memory through malloc(9), we always expect the amount of
 memory requested to be unsigned as a negative value would either stand for
 an error or an overflow.
 Unsign some values, found when considering the use of mallocarray(9), to
 avoid unnecessary casting. Also consider that indexes should be of
 at least the same size/type as the upper limit they pretend to index.


This might not break much, but it adds many more type errors and bogus
(implicit) casts than it fixes.  It actually changes the brokenness of the
first variable touched:


Modified: head/sys/fs/ext2fs/ext2_lookup.c
==
--- head/sys/fs/ext2fs/ext2_lookup.cWed Jan 24 17:52:06 2018
(r328345)
+++ head/sys/fs/ext2fs/ext2_lookup.cWed Jan 24 17:58:48 2018
(r328346)
@@ -145,9 +145,9 @@ ext2_readdir(struct vop_readdir_args *ap)
off_t offset, startoffset;
size_t readcnt, skipcnt;
ssize_t startresid;
-   int ncookies;
int DIRBLKSIZ = VTOI(ap->a_vp)->i_e2fs->e2fs_bsize;
int error;
+   u_int ncookies;


The first bug is only a style bug (unsorting the u_int after the ints even
when its variable was accidentally already before the ints.



if (uio->uio_offset < 0)
return (EINVAL);


That is the only change in this file.  No comment on other changes.

ncookies is mostly used in contexts other than for multiplying by it before
calling malloc(), and its type is now inconsistent with most other places.

The other places are:

X   ncookies = uio->uio_resid;

This has a more serious type error and consequent overflow bugs.  uio_resid
used to have type int, and cookies had to have type int to match that, else
there overflow occurs before the bounds checks.  Now uio_resid has type
ssize_t, which is excessively large on 64-bit arches (64 bits then), so the
assignment overflowed when ncookies had type int and uio_resid > INT_MAX.
Now it overflows differently when uio_resid > UINT_MAX, and unsign extension
causes overflow when uio_resid < 0.  There might be a sanity check on
uio_resid at higher levels, but I can only find a check related to EOF in
vfs_read_dirent().

Next, we do some bounds checking which seems to be correct modulo previous
overflows, and show the care needed for unsigned variables (i_size is unsigned
and must be compared with uio_offset which is signed).

Next, we assign ncookies, to ap_ncookies which still has the correct type
(plain signed int).  If ncookies were actually large enough to need a u_int,
or worse a 64-bit ssize_t, then this would overflow.

Later, we KASSERT() that ncookies > 0.  This might cause a compiler warning
"unsigned comparison with 0" now that ncookies is unsigned.  We count down
ncookies, but the loop termination condition is complicated and we don't
get any benefits from the possible micro-optimization of using ncookies as
a loop counter that counts down to 0.

To fix the problem that mallocarray() wanted a size_t arg, ncookies could
be cast to size_t, but that would be silly.  The prototype does the same
cast automatically even for cases with sign mismatches.  Now malloc()
wants a type of size_t (after further breakage to change malloc()s arg
type).  Many conversions are still involved, and casting would at most limit
compiler warnings:  the code is now:

X   malloc(ncookies * sizeof(*cookies), ...)

First, ncookies and sizeof(...) are promoted to a common type.  When ncookies
was int, usually it was promoted to size_t and sizeof(...) was not promoted.
But on exotic arches with size_t smaller than int, sizeof(...) is promoted to
int and ncookies is not promoted.

Next, the type of the result of the multiplication is the common type.

Finally, the prototype used to convert to u_long, but now converts to size_t.
When the common type is size_t, then the conversion is now null.  When the
common type was int, the conversion was promotion to u_long, but it is now
demotion to size_t.

All this only obviously works in practice because all the variables and
the product are small, so they are smaller than all of INT_MAX, SIZE_MAX
and ULONG_MAX on all supported and unsupported arches.  However, it is
hard to write bounds checks that obviously handle all cases, except
by using simple small bounds on the variables.

It is now obvious that the bounds checking is broken for general use.
There is no obvious limit on the malloc() except the file size.
However, I think cookies are only used by nfs, so this is not a
user-serving bug.  nfs just has to limit its cookie size.  It seems to
use a limit of something like NFS_SRVMAXIO (128K).  This also makes
the overflow from the ssize_t type error unreachable.  Note that it

So all of this actually works very unobviously in practice.  The sizes
are only small because nfs is the only (?) caller 

svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Pedro F. Giffuni]

Author: pfg
Date: Wed Jan 24 17:58:48 2018
New Revision: 328346
URL: https://svnweb.freebsd.org/changeset/base/328346

Log:
  ext2fs|ufs:Unsign some values related to allocation.
  
  When allocating memory through malloc(9), we always expect the amount of
  memory requested to be unsigned as a negative value would either stand for
  an error or an overflow.
  Unsign some values, found when considering the use of mallocarray(9), to
  avoid unnecessary casting. Also consider that indexes should be of
  at least the same size/type as the upper limit they pretend to index.
  
  MFC after:2 weeks

Modified:
  head/sys/fs/ext2fs/ext2_lookup.c
  head/sys/ufs/ffs/ffs_softdep.c
  head/sys/ufs/ufs/ufs_dirhash.c
  head/sys/ufs/ufs/ufs_vnops.c

Modified: head/sys/fs/ext2fs/ext2_lookup.c
==
--- head/sys/fs/ext2fs/ext2_lookup.cWed Jan 24 17:52:06 2018
(r328345)
+++ head/sys/fs/ext2fs/ext2_lookup.cWed Jan 24 17:58:48 2018
(r328346)
@@ -145,9 +145,9 @@ ext2_readdir(struct vop_readdir_args *ap)
off_t offset, startoffset;
size_t readcnt, skipcnt;
ssize_t startresid;
-   int ncookies;
int DIRBLKSIZ = VTOI(ap->a_vp)->i_e2fs->e2fs_bsize;
int error;
+   u_int ncookies;
 
if (uio->uio_offset < 0)
return (EINVAL);

Modified: head/sys/ufs/ffs/ffs_softdep.c
==
--- head/sys/ufs/ffs/ffs_softdep.c  Wed Jan 24 17:52:06 2018
(r328345)
+++ head/sys/ufs/ffs/ffs_softdep.c  Wed Jan 24 17:58:48 2018
(r328346)
@@ -2466,7 +2466,8 @@ softdep_mount(devvp, mp, fs, cred)
struct ufsmount *ump;
struct cg *cgp;
struct buf *bp;
-   int i, error, cyl;
+   u_int cyl, i;
+   int error;
 
sdp = malloc(sizeof(struct mount_softdeps), M_MOUNTDATA,
M_WAITOK | M_ZERO);

Modified: head/sys/ufs/ufs/ufs_dirhash.c
==
--- head/sys/ufs/ufs/ufs_dirhash.c  Wed Jan 24 17:52:06 2018
(r328345)
+++ head/sys/ufs/ufs/ufs_dirhash.c  Wed Jan 24 17:58:48 2018
(r328346)
@@ -349,7 +349,8 @@ ufsdirhash_build(struct inode *ip)
struct direct *ep;
struct vnode *vp;
doff_t bmask, pos;
-   int dirblocks, i, j, memreqd, nblocks, narrays, nslots, slot;
+   u_int dirblocks, i, narrays, nblocks, nslots;
+   int j, memreqd, slot;
 
/* Take care of a decreased sysctl value. */
while (ufs_dirhashmem > ufs_dirhashmaxmem) {

Modified: head/sys/ufs/ufs/ufs_vnops.c
==
--- head/sys/ufs/ufs/ufs_vnops.cWed Jan 24 17:52:06 2018
(r328345)
+++ head/sys/ufs/ufs/ufs_vnops.cWed Jan 24 17:58:48 2018
(r328346)
@@ -2170,7 +2170,7 @@ ufs_readdir(ap)
off_t offset, startoffset;
size_t readcnt, skipcnt;
ssize_t startresid;
-   int ncookies;
+   u_int ncookies;
int error;
 
if (uio->uio_offset < 0)
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"