"Nicholas A. Bellinger" <nab@xxxxxxxxxxxxxxx> writes: > Hi Dmitry, > > On Fri, 2017-03-31 at 19:53 +0400, Dmitry Monakhov wrote: >> If we failed to read data from backing file (probably because some one >> truncate file under us), we must zerofill cmd's data, otherwise it will >> be returned as is. Most likely cmd's data are unitialized pages from >> page cache. This result in information leak. >> >> testcase: https://github.com/dmonakhov/xfstests/commit/e11a1b7b907ca67b1be51a1594025600767366d5 >> Signed-off-by: Dmitry Monakhov <dmonakhov@xxxxxxxxxx> >> --- >> drivers/target/target_core_file.c | 21 +++++++++++++++------ >> 1 file changed, 15 insertions(+), 6 deletions(-) >> > > Nice catch on this one. Just a small comment below. > >> diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c >> index 87aa376..d69908d 100644 >> --- a/drivers/target/target_core_file.c >> +++ b/drivers/target/target_core_file.c > > <SNIP> > >> @@ -295,17 +294,27 @@ static int fd_do_rw(struct se_cmd *cmd, struct file *fd, >> pr_err("%s() returned %d, expecting %u for " >> "S_ISBLK\n", __func__, ret, >> data_length); >> - return (ret < 0 ? ret : -EINVAL); >> + if (ret >= 0) >> + ret = -EINVAL; >> } >> } else { >> if (ret < 0) { >> pr_err("%s() returned %d for non S_ISBLK\n", >> __func__, ret); >> - return ret; >> + } else if (ret != data_length) { >> + /* >> + * Short read case: >> + * Probably some one truncate file under us. >> + * We must explicitly zero sg-pages to prevent >> + * expose uninizialized pages to userspace. >> + */ >> + BUG_ON(ret > data_length); >> + ret += iov_iter_zero(data_length - ret, &iter); >> } > > A BUG_ON for this is overkill. No need to kill the whole node. ;) > > Applying + squashing the follow atop your original patch to just return > -EINVAL and fail se_cmd instead. The reason I've added bug_on here is that ret > data_length simply means that backed layer rewrite some random memory/disk beyond valid bvec boundaries and this is very bad sign. Even more, w/o any check my code makes things even more damaging because transfer off-by-one error on vfs layer to iov_iter_zero(-1, &iter), which result in zeroing 4Gb memory :) So check + EINVAL may be reasonable choice. > > Thank you, > > diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c > index d69908d..dd8f320 100644 > --- a/drivers/target/target_core_file.c > +++ b/drivers/target/target_core_file.c > @@ -308,8 +308,10 @@ static int fd_do_rw(struct se_cmd *cmd, struct file *fd, > * We must explicitly zero sg-pages to prevent > * expose uninizialized pages to userspace. > */ > - BUG_ON(ret > data_length); > - ret += iov_iter_zero(data_length - ret, &iter); > + if (ret < data_length) > + ret += iov_iter_zero(data_length - ret, &iter); > + else > + ret = -EINVAL; > } > } > } -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
