[PATCH] fs: sysfs: don't pass count == 0 to bin file readers

[PATCH] fs: sysfs: don't pass count == 0 to bin file readers

[Vladimir Zapolskiy]

If count == 0 bytes are requested by a reader, sysfs_kf_bin_read()
deliberately returns 0 without passing a potentially harmful value to
some externally defined underlying battr->read() function.

However in case of (pos == size && count) the next clause always sets
count to 0 and this value is handed over to battr->read().

The change intends to make obsolete (and remove later) a redundant
sanity check in battr->read(), if it is present, or add more
protection to struct bin_attribute users, who does not care about
input arguments.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 fs/sysfs/file.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 7c2867b..6c95628 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -90,7 +90,7 @@ static ssize_t sysfs_kf_bin_read(struct kernfs_open_file *of, char *buf,
 		return 0;
 
 	if (size) {
-		if (pos > size)
+		if (pos >= size)
 			return 0;
 		if (pos + count > size)
 			count = size - pos;
-- 
1.7.10.4

Re: [PATCH] fs: sysfs: don't pass count == 0 to bin file readers

[Tejun Heo]

Hello,

On Fri, May 22, 2015 at 12:21:16AM +0300, Vladimir Zapolskiy wrote:
> If count == 0 bytes are requested by a reader, sysfs_kf_bin_read()
> deliberately returns 0 without passing a potentially harmful value to
> some externally defined underlying battr->read() function.
> 
> However in case of (pos == size && count) the next clause always sets
> count to 0 and this value is handed over to battr->read().
> 
> The change intends to make obsolete (and remove later) a redundant
> sanity check in battr->read(), if it is present, or add more
> protection to struct bin_attribute users, who does not care about
> input arguments.
> 
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
>  fs/sysfs/file.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index 7c2867b..6c95628 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -90,7 +90,7 @@ static ssize_t sysfs_kf_bin_read(struct kernfs_open_file *of, char *buf,
>  		return 0;
>  
>  	if (size) {
> -		if (pos > size)
> +		if (pos >= size)
>  			return 0;
>  		if (pos + count > size)
>  			count = size - pos;

Hmmm... maybe just move that test upwards?

	if (!count || pos >= size)
		return 0;

	count = min(count, size - pos);

-- 
tejun

Re: [PATCH] fs: sysfs: don't pass count == 0 to bin file readers

[Vladimir Zapolskiy]

Hello Tejun,

On 22.05.2015 01:14, Tejun Heo wrote:
> Hello,
> 
> On Fri, May 22, 2015 at 12:21:16AM +0300, Vladimir Zapolskiy wrote:
>> If count == 0 bytes are requested by a reader, sysfs_kf_bin_read()
>> deliberately returns 0 without passing a potentially harmful value to
>> some externally defined underlying battr->read() function.
>>
>> However in case of (pos == size && count) the next clause always sets
>> count to 0 and this value is handed over to battr->read().
>>
>> The change intends to make obsolete (and remove later) a redundant
>> sanity check in battr->read(), if it is present, or add more
>> protection to struct bin_attribute users, who does not care about
>> input arguments.
>>
>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>> ---
>>  fs/sysfs/file.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
>> index 7c2867b..6c95628 100644
>> --- a/fs/sysfs/file.c
>> +++ b/fs/sysfs/file.c
>> @@ -90,7 +90,7 @@ static ssize_t sysfs_kf_bin_read(struct kernfs_open_file *of, char *buf,
>>  		return 0;
>>  
>>  	if (size) {
>> -		if (pos > size)
>> +		if (pos >= size)
>>  			return 0;
>>  		if (pos + count > size)
>>  			count = size - pos;
> 
> Hmmm... maybe just move that test upwards?
> 
> 	if (!count || pos >= size)
> 		return 0;
> 
> 	count = min(count, size - pos);
> 

If the code block stays within if (size && count) { ... }, then !count
check is redundant (you may notice that !count check is already present
above but not shown in diff's 3 lines context), and I agree that

	if (pos >= size)
		return 0;

	if (pos + count > size)
		count = size - pos;

and

	if (pos >= size)
		return 0;

	count = min(count, size - pos);

are equal.

But "!size" is a special case,

	if (!count || pos >= size)
		return 0;

seems to be incorrect in case of !size ===> (pos >= size) == true.

To the sent change I may add a replacement of "if (pos + count > size)
..." with min_t (ssize_t, count, size - pos), if you wish.

--
With best wishes,
Vladimir

Re: [PATCH] fs: sysfs: don't pass count == 0 to bin file readers

[Tejun Heo]

Hello, Vladimir.

On Fri, May 22, 2015 at 02:04:31AM +0300, Vladimir Zapolskiy wrote:
> But "!size" is a special case,
> 
> 	if (!count || pos >= size)
> 		return 0;
> 
> seems to be incorrect in case of !size ===> (pos >= size) == true.

Hmmm... Why is that wrong tho?  If size is zero and pos is zero,
there's nothing to do, no?

Thanks.

-- 
tejun

Re: [PATCH] fs: sysfs: don't pass count == 0 to bin file readers

[Vladimir Zapolskiy]

Hello Tejun,

On 22.05.2015 02:26, Tejun Heo wrote:
> Hello, Vladimir.
> 
> On Fri, May 22, 2015 at 02:04:31AM +0300, Vladimir Zapolskiy wrote:
>> But "!size" is a special case,
>>
>> 	if (!count || pos >= size)
>> 		return 0;
>>
>> seems to be incorrect in case of !size ===> (pos >= size) == true.
> 
> Hmmm... Why is that wrong tho?  If size is zero and pos is zero,
> there's nothing to do, no?

positive size value in the context means the fixed exact length of the
file and if size == 0, then it represents some undefined size, often
dynamic in runtime. So, if size is zero and pos is zero it stands for
reading from the beginning of the file as many bytes as allowed by
battr->read() realization. This special case is utilized by quite many
bin_attribute users, probably more than half of them set .size to 0.

> Thanks.
> 

--
With best wishes,
Vladimir

Re: [PATCH] fs: sysfs: don't pass count == 0 to bin file readers

[Tejun Heo]

On Fri, May 22, 2015 at 03:46:51AM +0300, Vladimir Zapolskiy wrote:
> Hello Tejun,
> 
> On 22.05.2015 02:26, Tejun Heo wrote:
> > Hello, Vladimir.
> > 
> > On Fri, May 22, 2015 at 02:04:31AM +0300, Vladimir Zapolskiy wrote:
> >> But "!size" is a special case,
> >>
> >> 	if (!count || pos >= size)
> >> 		return 0;
> >>
> >> seems to be incorrect in case of !size ===> (pos >= size) == true.
> > 
> > Hmmm... Why is that wrong tho?  If size is zero and pos is zero,
> > there's nothing to do, no?
> 
> positive size value in the context means the fixed exact length of the
> file and if size == 0, then it represents some undefined size, often
> dynamic in runtime. So, if size is zero and pos is zero it stands for
> reading from the beginning of the file as many bytes as allowed by
> battr->read() realization. This special case is utilized by quite many
> bin_attribute users, probably more than half of them set .size to 0.

Ah, right, I completely forgot about that.  Please feel free to add

 Acked-by: Tejun Heo <tj@kernel.org>

to the original patch.

Thanks for the patience.

-- 
tejun