| Message ID | 20200615121257.798894-11-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/13] cachefiles: switch to kernel_write | expand |
On Mon, Jun 15, 2020 at 5:13 AM Christoph Hellwig <hch@lst.de> wrote: > > __kernel_read has a bunch of additional sanity checks, and this moves > the set_fs out of non-core code. Wel, you also seem to be removing this part: > - if (!(file->f_mode & FMODE_READ)) > - return -EBADF; which you didn't add in the previous patch that implemented __kernel_read(). It worries me that you're making these kinds of transformations where the comments imply it's a no-op, but the actual code doesn't agree. Especially when it's part of one large patch series and each commit looks trivial. This kind of series needs more care. Maybe that test isn't necessary, but it isn't obvious, and I really don't like how you completely glossed over totally changing what the code did. Linus
On Mon, Jun 15, 2020 at 9:46 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > It worries me that you're making these kinds of transformations where > the comments imply it's a no-op, but the actual code doesn't agree. Note that it's not that I think the FMODE_READ check is necessarily _needed_. It's more the discrepancy between the commit message and the code change that I don't like. The commit message implies that __kernel_read() has _more_ checks than the checks done by integrity_kernel_read(). But it looks like they aren't so much "more" as they are just "different". Linus
diff --git a/security/integrity/iint.c b/security/integrity/iint.c index e12c4900510f60..1d20003243c3fb 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -188,19 +188,7 @@ DEFINE_LSM(integrity) = { int integrity_kernel_read(struct file *file, loff_t offset, void *addr, unsigned long count) { - mm_segment_t old_fs; - char __user *buf = (char __user *)addr; - ssize_t ret; - - if (!(file->f_mode & FMODE_READ)) - return -EBADF; - - old_fs = get_fs(); - set_fs(KERNEL_DS); - ret = __vfs_read(file, buf, count, &offset); - set_fs(old_fs); - - return ret; + return __kernel_read(file, addr, count, &offset); } /*
__kernel_read has a bunch of additional sanity checks, and this moves the set_fs out of non-core code. Signed-off-by: Christoph Hellwig <hch@lst.de> --- security/integrity/iint.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-)