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

[Bruce Evans]

On Sat, 27 Jan 2018, Konstantin Belousov wrote:


On Sat, Jan 27, 2018 at 03:33:52PM +, Pedro F. Giffuni wrote:

Log:
  {ext2|ufs}_readdir: Set limit on valid ncookies values.

  Sanitize the values that will be assigned to ncookies so that we ensure
  they are sane and we can handle them.

  Let ncookies signed as it was before r328346. The valid range is such
  that unsigned values are not required and we are not able to avoid at
  least one cast anyways.

  Hinted by:bde

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

Modified: head/sys/fs/ext2fs/ext2_lookup.c
==
--- head/sys/fs/ext2fs/ext2_lookup.cSat Jan 27 13:46:55 2018
(r328478)
+++ head/sys/fs/ext2fs/ext2_lookup.cSat Jan 27 15:33:52 2018
(r328479)
@@ -145,14 +145,18 @@ ext2_readdir(struct vop_readdir_args *ap)
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;

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;

Modified: head/sys/ufs/ufs/ufs_vnops.c
...

You just break nfs server.

Look at nfsrvd_readdir() call to VOP_READDIR.  Almost all code which calls
VOP_READDIR() memoize the value of uio_resid before and after the call and
interpret the difference as the amount of data returned.


We noticed in further discussion that there is likely to be a problem like
that.

Also, the instructions for testing this said to test nfs with a value of
64K and perhaps with smaller values, to see if its error handling can
handle reduction of uio_resid.  Any testing would have shown the problem.


I said above that only nfs server is broken, because only the server uses
cookies, otherwise your patch would break everything.


My original patch have broke syscalls slightly too.  In the syscall case,
it only modifies uio_resid when that is negative.  Spelling 0 as negative
would be silly, but it has always worked, and fixing it up to 0 would
break callers which memoized the negative value.

I think we have use a local variable to hold a reduced resid.  uio_resid
can be updated just before returning.  Or maybe just return EINVAL in the
cookies case if uio_resid < 0 || uio_resid > MAX_SIZE_NOW_USED_BY_NFS
(thiss essentially a check that nfs is the only caller and that it never
asks for large sizes).  I was trying to avoid the extra complexity.  There
are at about 13 file systems under sys/fs, and zfs under sys/cddl to fix.

zfs_readdir() is quite broken (if there are any untrusted callers).  It
always malloc()s approx. (size_t)(int)uio_resid bytes.  The casts can
overflow in various ways.  Some other file systems resuce this to the
residual file size which tends to be small enough to fit in memory.
zfs detects overrun (using KASSERT()) _after_ the overrun has occurred.

The subsystems under sys/fs which mention ncookies are:
- autofs: just for layering
- cd9660: like zfs, except it has the wrong type (u_int) for the cookie
  count, and it has no KASSERT() at all, and it counts down
  the residual cookies quite differently
- devfs: doesn't support cookies, but fakes them a little.  No problem.
- fdesc: doesn't support cookies.  No problem.
- fuse: doesn't support cookies.  Doesn't even return a cookie count of 0
  to advertize this like fdesc does.
- msdosfs: like cd9660, except it counts down the residual cookies normally
- smbfs: doesn't support cookies.  Has some code to return EOPNOTSUPP if
  cookies are requested, but this is ifdefed out, so it is like fuse.
- tmpfs: tmpfs_subr.c writes cookies and depends on the caller passing
  sane cookie pointer and count args.  tmpfs_readdir() creates a cookie
  array with a size that seems to be for a whole directory (not limited
  by uio_resid).  So nfs's saneness doesn't help here.  I don't know how
  large tmpfs directories can be (can it be swap-backed with directories
  larger than memory?).
- udf: like cd9660.
- unionfs: does both layering and a malloc() to merge cookies.  The
  size of this malloc() is unclear.

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

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

[Pedro Giffuni]

On 01/27/18 11:03, Konstantin Belousov wrote:

On Sat, Jan 27, 2018 at 03:33:52PM +, Pedro F. Giffuni wrote:

Author: pfg
Date: Sat Jan 27 15:33:52 2018
New Revision: 328479
URL: https://svnweb.freebsd.org/changeset/base/328479

Log:
   {ext2|ufs}_readdir: Set limit on valid ncookies values.
   
   Sanitize the values that will be assigned to ncookies so that we ensure

   they are sane and we can handle them.
   
   Let ncookies signed as it was before r328346. The valid range is such

   that unsigned values are not required and we are not able to avoid at
   least one cast anyways.
   
   Hinted by:	bde


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

Modified: head/sys/fs/ext2fs/ext2_lookup.c
==
--- head/sys/fs/ext2fs/ext2_lookup.cSat Jan 27 13:46:55 2018
(r328478)
+++ head/sys/fs/ext2fs/ext2_lookup.cSat Jan 27 15:33:52 2018
(r328479)
@@ -145,14 +145,18 @@ ext2_readdir(struct vop_readdir_args *ap)
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;
  
  	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;

Modified: head/sys/ufs/ufs/ufs_vnops.c
==
--- head/sys/ufs/ufs/ufs_vnops.cSat Jan 27 13:46:55 2018
(r328478)
+++ head/sys/ufs/ufs/ufs_vnops.cSat Jan 27 15:33:52 2018
(r328479)
@@ -2170,7 +2170,7 @@ ufs_readdir(ap)
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 @@ ufs_readdir(ap)
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;

You just break nfs server.

Look at nfsrvd_readdir() call to VOP_READDIR.  Almost all code which calls
VOP_READDIR() memoize the value of uio_resid before and after the call and
interpret the difference as the amount of data returned.

I said above that only nfs server is broken, because only the server uses
cookies, otherwise your patch would break everything.
Ugh, yes .. I completely missed the fact that uio is a pointer to the 
real thing, not a local copy.

This still should never go off limits but it is certainly wrong.

I reverted it sorry.

Pedro.

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

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

[Warner Losh]

You aren't allowed to set resid like this. Changes in resid indicate amount
of I/O done. If you think it's bogus, you need to either return EINVAL or
use a smaller value to figure out your buffer sizes. Thi s is bogus, please
back it out.

Warner

On Jan 27, 2018 8:34 AM, "Pedro F. Giffuni"  wrote:

> Author: pfg
> Date: Sat Jan 27 15:33:52 2018
> New Revision: 328479
> URL: https://svnweb.freebsd.org/changeset/base/328479
>
> Log:
>   {ext2|ufs}_readdir: Set limit on valid ncookies values.
>
>   Sanitize the values that will be assigned to ncookies so that we ensure
>   they are sane and we can handle them.
>
>   Let ncookies signed as it was before r328346. The valid range is such
>   that unsigned values are not required and we are not able to avoid at
>   least one cast anyways.
>
>   Hinted by:bde
>
> Modified:
>   head/sys/fs/ext2fs/ext2_lookup.c
>   head/sys/ufs/ufs/ufs_vnops.c
>
> Modified: head/sys/fs/ext2fs/ext2_lookup.c
> 
> ==
> --- head/sys/fs/ext2fs/ext2_lookup.cSat Jan 27 13:46:55 2018
> (r328478)
> +++ head/sys/fs/ext2fs/ext2_lookup.cSat Jan 27 15:33:52 2018
> (r328479)
> @@ -145,14 +145,18 @@ ext2_readdir(struct vop_readdir_args *ap)
> 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;
>
> 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;
>
> Modified: head/sys/ufs/ufs/ufs_vnops.c
> 
> ==
> --- head/sys/ufs/ufs/ufs_vnops.cSat Jan 27 13:46:55 2018
> (r328478)
> +++ head/sys/ufs/ufs/ufs_vnops.cSat Jan 27 15:33:52 2018
> (r328479)
> @@ -2170,7 +2170,7 @@ ufs_readdir(ap)
> 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 @@ ufs_readdir(ap)
> 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-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Konstantin Belousov]

On Sat, Jan 27, 2018 at 03:33:52PM +, Pedro F. Giffuni wrote:
> Author: pfg
> Date: Sat Jan 27 15:33:52 2018
> New Revision: 328479
> URL: https://svnweb.freebsd.org/changeset/base/328479
> 
> Log:
>   {ext2|ufs}_readdir: Set limit on valid ncookies values.
>   
>   Sanitize the values that will be assigned to ncookies so that we ensure
>   they are sane and we can handle them.
>   
>   Let ncookies signed as it was before r328346. The valid range is such
>   that unsigned values are not required and we are not able to avoid at
>   least one cast anyways.
>   
>   Hinted by:  bde
> 
> Modified:
>   head/sys/fs/ext2fs/ext2_lookup.c
>   head/sys/ufs/ufs/ufs_vnops.c
> 
> Modified: head/sys/fs/ext2fs/ext2_lookup.c
> ==
> --- head/sys/fs/ext2fs/ext2_lookup.c  Sat Jan 27 13:46:55 2018
> (r328478)
> +++ head/sys/fs/ext2fs/ext2_lookup.c  Sat Jan 27 15:33:52 2018
> (r328479)
> @@ -145,14 +145,18 @@ ext2_readdir(struct vop_readdir_args *ap)
>   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;
>  
>   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;
> 
> Modified: head/sys/ufs/ufs/ufs_vnops.c
> ==
> --- head/sys/ufs/ufs/ufs_vnops.c  Sat Jan 27 13:46:55 2018
> (r328478)
> +++ head/sys/ufs/ufs/ufs_vnops.c  Sat Jan 27 15:33:52 2018
> (r328479)
> @@ -2170,7 +2170,7 @@ ufs_readdir(ap)
>   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 @@ ufs_readdir(ap)
>   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;
You just break nfs server.

Look at nfsrvd_readdir() call to VOP_READDIR.  Almost all code which calls
VOP_READDIR() memoize the value of uio_resid before and after the call and
interpret the difference as the amount of data returned.

I said above that only nfs server is broken, because only the server uses
cookies, otherwise your patch would break everything.

>   ncookies = uio->uio_resid;
>   if (uio->uio_offset >= ip->i_size)
>   ncookies = 0;
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

svn commit: r328479 - in head/sys: fs/ext2fs ufs/ufs

[Pedro F. Giffuni]

Author: pfg
Date: Sat Jan 27 15:33:52 2018
New Revision: 328479
URL: https://svnweb.freebsd.org/changeset/base/328479

Log:
  {ext2|ufs}_readdir: Set limit on valid ncookies values.
  
  Sanitize the values that will be assigned to ncookies so that we ensure
  they are sane and we can handle them.
  
  Let ncookies signed as it was before r328346. The valid range is such
  that unsigned values are not required and we are not able to avoid at
  least one cast anyways.
  
  Hinted by:bde

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

Modified: head/sys/fs/ext2fs/ext2_lookup.c
==
--- head/sys/fs/ext2fs/ext2_lookup.cSat Jan 27 13:46:55 2018
(r328478)
+++ head/sys/fs/ext2fs/ext2_lookup.cSat Jan 27 15:33:52 2018
(r328479)
@@ -145,14 +145,18 @@ ext2_readdir(struct vop_readdir_args *ap)
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;
 
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;

Modified: head/sys/ufs/ufs/ufs_vnops.c
==
--- head/sys/ufs/ufs/ufs_vnops.cSat Jan 27 13:46:55 2018
(r328478)
+++ head/sys/ufs/ufs/ufs_vnops.cSat Jan 27 15:33:52 2018
(r328479)
@@ -2170,7 +2170,7 @@ ufs_readdir(ap)
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 @@ ufs_readdir(ap)
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-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"