On 01/27/18 11:14, Warner Losh wrote:
On Jan 27, 2018 8:17 AM, "Pedro Giffuni" <p...@freebsd.org
<mailto:p...@freebsd.org>> 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-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"
