| Message ID | 20170816173615.10098-1-ross.zwisler@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed 16-08-17 11:36:15, Ross Zwisler wrote: > Add a comment explaining how the user addresses provided to read(2) and > write(2) are validated in the DAX I/O path. We call dax_copy_from_iter() > or copy_to_iter() on these without calling access_ok() first in the DAX > code, and there was a concern that the user might be able to read/write to > arbitrary kernel addresses with this path. > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> Looks OK to me so feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Just I'd note that standard buffered read / write path is no different so I don't see a big point in adding this comment when it is not in any other path either... Honza > --- > > Adding a comment instead of adding redundant access_ok() calls in the DAX > code. If this is the wrong path to take, please let me know. > > fs/dax.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/fs/dax.c b/fs/dax.c > index 8c67517..2d50f32 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -1060,6 +1060,11 @@ dax_iomap_actor(struct inode *inode, loff_t pos, loff_t length, void *data, > if (map_len > end - pos) > map_len = end - pos; > > + /* > + * The userspace address for the memory copy has already been > + * validated via access_ok() in either vfs_read() or > + * vfs_write(), depending on which operation we are doing. > + */ > if (iov_iter_rw(iter) == WRITE) > map_len = dax_copy_from_iter(dax_dev, pgoff, kaddr, > map_len, iter); > -- > 2.9.5 >
On Thu, Aug 17, 2017 at 10:53:32AM +0200, Jan Kara wrote: > On Wed 16-08-17 11:36:15, Ross Zwisler wrote: > > Add a comment explaining how the user addresses provided to read(2) and > > write(2) are validated in the DAX I/O path. We call dax_copy_from_iter() > > or copy_to_iter() on these without calling access_ok() first in the DAX > > code, and there was a concern that the user might be able to read/write to > > arbitrary kernel addresses with this path. > > > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > Looks OK to me so feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> > > Just I'd note that standard buffered read / write path is no different so I > don't see a big point in adding this comment when it is not in any other > path either... Fair enough. Yea, if it's not in any of the other paths either and it's just common knowledge that these addresses are validated at the VFS layer, we can leave it out.
diff --git a/fs/dax.c b/fs/dax.c index 8c67517..2d50f32 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -1060,6 +1060,11 @@ dax_iomap_actor(struct inode *inode, loff_t pos, loff_t length, void *data, if (map_len > end - pos) map_len = end - pos; + /* + * The userspace address for the memory copy has already been + * validated via access_ok() in either vfs_read() or + * vfs_write(), depending on which operation we are doing. + */ if (iov_iter_rw(iter) == WRITE) map_len = dax_copy_from_iter(dax_dev, pgoff, kaddr, map_len, iter);
Add a comment explaining how the user addresses provided to read(2) and write(2) are validated in the DAX I/O path. We call dax_copy_from_iter() or copy_to_iter() on these without calling access_ok() first in the DAX code, and there was a concern that the user might be able to read/write to arbitrary kernel addresses with this path. Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> --- Adding a comment instead of adding redundant access_ok() calls in the DAX code. If this is the wrong path to take, please let me know. fs/dax.c | 5 +++++ 1 file changed, 5 insertions(+)