[PATCH 1/1] ima: fix fallback to use new_sync_read()

[PATCH 1/1] ima: fix fallback to use new_sync_read()

[Dmitry Kasatkin]

3.16 commit aad4f8bb42af06371aa0e85bf0cd9d52c0494985
'switch simple generic_file_aio_read() users to ->read_iter()'
replaced ->aio_read with ->read_iter in most of the file systems
and introduced new_sync_read() as a replacement for do_sync_read().

Most of file systems set '->read' and ima_kernel_read is not affected.
When ->read is not set, this patch adopts fallback call changes from the
vfs_read.

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 security/integrity/ima/ima_crypto.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index ccd0ac8..b126a78 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -40,19 +40,19 @@ static int ima_kernel_read(struct file *file, loff_t offset,
 {
 	mm_segment_t old_fs;
 	char __user *buf = addr;
-	ssize_t ret;
+	ssize_t ret = -EINVAL;
 
 	if (!(file->f_mode & FMODE_READ))
 		return -EBADF;
-	if (!file->f_op->read && !file->f_op->aio_read)
-		return -EINVAL;
 
 	old_fs = get_fs();
 	set_fs(get_ds());
 	if (file->f_op->read)
 		ret = file->f_op->read(file, buf, count, &offset);
-	else
+	else if (file->f_op->aio_read)
 		ret = do_sync_read(file, buf, count, &offset);
+	else if (file->f_op->read_iter)
+		ret = new_sync_read(file, buf, count, &offset);
 	set_fs(old_fs);
 	return ret;
 }
-- 
1.9.1

Re: [PATCH 1/1] ima: fix fallback to use new_sync_read()

[Mimi Zohar]

On Tue, 2014-06-24 at 16:27 +0300, Dmitry Kasatkin wrote: 
> 3.16 commit aad4f8bb42af06371aa0e85bf0cd9d52c0494985
> 'switch simple generic_file_aio_read() users to ->read_iter()'
> replaced ->aio_read with ->read_iter in most of the file systems
> and introduced new_sync_read() as a replacement for do_sync_read().
> 
> Most of file systems set '->read' and ima_kernel_read is not affected.
> When ->read is not set, this patch adopts fallback call changes from the
> vfs_read.

So every time there are changes in vfs_read(), we're going to have to
play catch up.  A better solution would be to refactor vfs_read() so
that we could call it.

Mimi


> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> ---
>  security/integrity/ima/ima_crypto.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index ccd0ac8..b126a78 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -40,19 +40,19 @@ static int ima_kernel_read(struct file *file, loff_t offset,
>  {
>  	mm_segment_t old_fs;
>  	char __user *buf = addr;
> -	ssize_t ret;
> +	ssize_t ret = -EINVAL;
> 
>  	if (!(file->f_mode & FMODE_READ))
>  		return -EBADF;
> -	if (!file->f_op->read && !file->f_op->aio_read)
> -		return -EINVAL;
> 
>  	old_fs = get_fs();
>  	set_fs(get_ds());
>  	if (file->f_op->read)
>  		ret = file->f_op->read(file, buf, count, &offset);
> -	else
> +	else if (file->f_op->aio_read)
>  		ret = do_sync_read(file, buf, count, &offset);
> +	else if (file->f_op->read_iter)
> +		ret = new_sync_read(file, buf, count, &offset);
>  	set_fs(old_fs);
>  	return ret;
>  }

Re: [PATCH 1/1] ima: fix fallback to use new_sync_read()

[Dmitry Kasatkin]

On 26/06/14 15:49, Mimi Zohar wrote:
> On Tue, 2014-06-24 at 16:27 +0300, Dmitry Kasatkin wrote: 
>> 3.16 commit aad4f8bb42af06371aa0e85bf0cd9d52c0494985
>> 'switch simple generic_file_aio_read() users to ->read_iter()'
>> replaced ->aio_read with ->read_iter in most of the file systems
>> and introduced new_sync_read() as a replacement for do_sync_read().
>>
>> Most of file systems set '->read' and ima_kernel_read is not affected.
>> When ->read is not set, this patch adopts fallback call changes from the
>> vfs_read.
> So every time there are changes in vfs_read(), we're going to have to
> play catch up.  A better solution would be to refactor vfs_read() so
> that we could call it.
>
> Mimi

vfs_read was stable for decade.
And next will be the same.

I followed the same approach we took to have an own version.

Refactoring vfs_read is not the best approach as we have set_fs things
there.

I would prefer to have "kernel_read_nosec" function along with
kernel_read/vfs_read, so changes could be
made noticeably together..

- Dmitry


>
>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
>> ---
>>  security/integrity/ima/ima_crypto.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
>> index ccd0ac8..b126a78 100644
>> --- a/security/integrity/ima/ima_crypto.c
>> +++ b/security/integrity/ima/ima_crypto.c
>> @@ -40,19 +40,19 @@ static int ima_kernel_read(struct file *file, loff_t offset,
>>  {
>>  	mm_segment_t old_fs;
>>  	char __user *buf = addr;
>> -	ssize_t ret;
>> +	ssize_t ret = -EINVAL;
>>
>>  	if (!(file->f_mode & FMODE_READ))
>>  		return -EBADF;
>> -	if (!file->f_op->read && !file->f_op->aio_read)
>> -		return -EINVAL;
>>
>>  	old_fs = get_fs();
>>  	set_fs(get_ds());
>>  	if (file->f_op->read)
>>  		ret = file->f_op->read(file, buf, count, &offset);
>> -	else
>> +	else if (file->f_op->aio_read)
>>  		ret = do_sync_read(file, buf, count, &offset);
>> +	else if (file->f_op->read_iter)
>> +		ret = new_sync_read(file, buf, count, &offset);
>>  	set_fs(old_fs);
>>  	return ret;
>>  }
>
>

Re: [PATCH 1/1] ima: fix fallback to use new_sync_read()

[Dmitry Kasatkin]

On 26/06/14 16:20, Dmitry Kasatkin wrote:
> On 26/06/14 15:49, Mimi Zohar wrote:
>> On Tue, 2014-06-24 at 16:27 +0300, Dmitry Kasatkin wrote: 
>>> 3.16 commit aad4f8bb42af06371aa0e85bf0cd9d52c0494985
>>> 'switch simple generic_file_aio_read() users to ->read_iter()'
>>> replaced ->aio_read with ->read_iter in most of the file systems
>>> and introduced new_sync_read() as a replacement for do_sync_read().
>>>
>>> Most of file systems set '->read' and ima_kernel_read is not affected.
>>> When ->read is not set, this patch adopts fallback call changes from the
>>> vfs_read.
>> So every time there are changes in vfs_read(), we're going to have to
>> play catch up.  A better solution would be to refactor vfs_read() so
>> that we could call it.
>>
>> Mimi
> vfs_read was stable for decade.
> And next will be the same.
>
> I followed the same approach we took to have an own version.
>
> Refactoring vfs_read is not the best approach as we have set_fs things
> there.
>
> I would prefer to have "kernel_read_nosec" function along with
> kernel_read/vfs_read, so changes could be
> made noticeably together..

Another thing is that we do not increment system counters.
That is may be not what public "__kernel_read" want to do...

- Dmitry

> - Dmitry
>
>
>>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
>>> ---
>>>  security/integrity/ima/ima_crypto.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
>>> index ccd0ac8..b126a78 100644
>>> --- a/security/integrity/ima/ima_crypto.c
>>> +++ b/security/integrity/ima/ima_crypto.c
>>> @@ -40,19 +40,19 @@ static int ima_kernel_read(struct file *file, loff_t offset,
>>>  {
>>>  	mm_segment_t old_fs;
>>>  	char __user *buf = addr;
>>> -	ssize_t ret;
>>> +	ssize_t ret = -EINVAL;
>>>
>>>  	if (!(file->f_mode & FMODE_READ))
>>>  		return -EBADF;
>>> -	if (!file->f_op->read && !file->f_op->aio_read)
>>> -		return -EINVAL;
>>>
>>>  	old_fs = get_fs();
>>>  	set_fs(get_ds());
>>>  	if (file->f_op->read)
>>>  		ret = file->f_op->read(file, buf, count, &offset);
>>> -	else
>>> +	else if (file->f_op->aio_read)
>>>  		ret = do_sync_read(file, buf, count, &offset);
>>> +	else if (file->f_op->read_iter)
>>> +		ret = new_sync_read(file, buf, count, &offset);
>>>  	set_fs(old_fs);
>>>  	return ret;
>>>  }
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/1] ima: fix fallback to use new_sync_read()

[Mimi Zohar]

On Thu, 2014-06-26 at 16:28 +0300, Dmitry Kasatkin wrote: 
> On 26/06/14 16:20, Dmitry Kasatkin wrote:
> > On 26/06/14 15:49, Mimi Zohar wrote:
> >> On Tue, 2014-06-24 at 16:27 +0300, Dmitry Kasatkin wrote: 
> >>> 3.16 commit aad4f8bb42af06371aa0e85bf0cd9d52c0494985
> >>> 'switch simple generic_file_aio_read() users to ->read_iter()'
> >>> replaced ->aio_read with ->read_iter in most of the file systems
> >>> and introduced new_sync_read() as a replacement for do_sync_read().
> >>>
> >>> Most of file systems set '->read' and ima_kernel_read is not affected.
> >>> When ->read is not set, this patch adopts fallback call changes from the
> >>> vfs_read.
> >> So every time there are changes in vfs_read(), we're going to have to
> >> play catch up.  A better solution would be to refactor vfs_read() so
> >> that we could call it.
> >>
> >> Mimi
> > vfs_read was stable for decade.
> > And next will be the same.
> >
> > I followed the same approach we took to have an own version.
> >
> > Refactoring vfs_read is not the best approach as we have set_fs things
> > there.
> >
> > I would prefer to have "kernel_read_nosec" function along with
> > kernel_read/vfs_read, so changes could be
> > made noticeably together..
> 
> Another thing is that we do not increment system counters.
> That is may be not what public "__kernel_read" want to do...

As reference, look at fs/read_write.c: vfs_write() and __kernel_write().

thanks,

Mimi