[PATCH] arch:powerpc simple_write_to_buffer return check

Signed-off-by: Mayank Suman <redacted>
---
 arch/powerpc/kernel/eeh.c                    | 8 ++++----
 arch/powerpc/platforms/powernv/eeh-powernv.c | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 813713c9120c..2dbe1558a71f 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c

@@ -1628,8 +1628,8 @@ static ssize_t eeh_force_recover_write(struct file *filp,
 	char buf[20];
 	int ret;
 
-	ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count);
-	if (!ret)
+	ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count);
+	if (ret <= 0)
 		return -EFAULT;
 
 	/*

@@ -1696,7 +1696,7 @@ static ssize_t eeh_dev_check_write(struct file *filp,
 
 	memset(buf, 0, sizeof(buf));
 	ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count);
-	if (!ret)
+	if (ret <= 0)
 		return -EFAULT;
 
 	ret = sscanf(buf, "%x:%x:%x.%x", &domain, &bus, &dev, &fn);

@@ -1836,7 +1836,7 @@ static ssize_t eeh_dev_break_write(struct file *filp,
 
 	memset(buf, 0, sizeof(buf));
 	ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count);
-	if (!ret)
+	if (ret <= 0)
 		return -EFAULT;
 
 	ret = sscanf(buf, "%x:%x:%x.%x", &domain, &bus, &dev, &fn);

diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
index 89e22c460ebf..36ed2b8f7375 100644
--- a/arch/powerpc/platforms/powernv/eeh-powernv.c
+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c

@@ -76,8 +76,8 @@ static ssize_t pnv_eeh_ei_write(struct file *filp,
 		return -ENXIO;
 
 	/* Copy over argument buffer */
-	ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count);
-	if (!ret)
+	ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count);
+	if (ret <= 0)
 		return -EFAULT;
 
 	/* Retrieve parameters */

-- 
2.30.0

On Fri, Feb 5, 2021 at 5:17 AM Mayank Suman [off-list ref] wrote:

Signed-off-by: Mayank Suman <redacted>

commit messages aren't optional

We should probably be zeroing the buffer. Reading to sizeof(buf) - 1
is done in a few places to guarantee that the string is nul
terminated, but without the preceeding memset() that isn't actually
guaranteed.

+       if (ret <= 0)
                return -EFAULT;

EFAULT is supposed to be returned when the user supplies a buffer to
write(2) which is outside their address space. I figured letting the
sscanf() in the next step fail if the user passes writes a zero-length
buffer and returning EINVAL made more sense. That said, the exact
semantics around zero length writes are pretty handwavy so I guess
this isn't wrong, but I don't think it's better either.

On 05/02/21 4:05 am, Oliver O'Halloran wrote:

On Fri, Feb 5, 2021 at 5:17 AM Mayank Suman [off-list ref] wrote:

quoted

Signed-off-by: Mayank Suman <redacted>

commit messages aren't optional

Sorry. I will include the commit message in PATCH v2.

quoted

---
 arch/powerpc/kernel/eeh.c                    | 8 ++++----
 arch/powerpc/platforms/powernv/eeh-powernv.c | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 813713c9120c..2dbe1558a71f 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c

@@ -1628,8 +1628,8 @@ static ssize_t eeh_force_recover_write(struct file *filp,
        char buf[20];
        int ret;

-       ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count);
-       if (!ret)
+       ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count);

We should probably be zeroing the buffer. Reading to sizeof(buf) - 1
is done in a few places to guarantee that the string is nul
terminated, but without the preceeding memset() that isn't actually
guaranteed.

Yes, the buffer should be zeroed out first. I have included memset() in Patch v2.

quoted

+       if (ret <= 0)
                return -EFAULT;

EFAULT is supposed to be returned when the user supplies a buffer to
write(2) which is outside their address space. I figured letting the
sscanf() in the next step fail if the user passes writes a zero-length
buffer and returning EINVAL made more sense. That said, the exact
semantics around zero length writes are pretty handwavy so I guess
this isn't wrong, but I don't think it's better either.

simple_write_to_buffer may return negative value on fail.
So, -EFAULT should be return in case of negative return value. 
The conditional (!ret) was not sufficient to catch negative return value.

Please provide some description of the change.

And please clarify the patch subject, because as far as I can see, the return is already checked 
allthough the check seams wrong.

Le 04/02/2021 à 19:16, Mayank Suman a écrit :

Why return -EFAULT when the function has returned -EINVAL ?
And why is it -EFAULT when ret is 0 ? EFAULT means error accessing memory.

On 05/02/21 12:51 pm, Christophe Leroy wrote:

Please provide some description of the change.

And please clarify the patch subject, because as far as I can see, the return is already checked allthough the check seams wrong.

This was my first patch. I will try to provide better description of changes and subject in later patches.

Le 04/02/2021 à 19:16, Mayank Suman a écrit :

quoted

Signed-off-by: Mayank Suman <redacted>
---
  arch/powerpc/kernel/eeh.c                    | 8 ++++----
  arch/powerpc/platforms/powernv/eeh-powernv.c | 4 ++--
  2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 813713c9120c..2dbe1558a71f 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c

@@ -1628,8 +1628,8 @@ static ssize_t eeh_force_recover_write(struct file *filp,

      char buf[20];
      int ret;
  -    ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count);
-    if (!ret)
+    ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count);
+    if (ret <= 0) >           return -EFAULT;

Why return -EFAULT when the function has returned -EINVAL ?

If -EINVAL is returned by simple_write_to_buffer, we should return -EINVAL.

And why is it -EFAULT when ret is 0 ? EFAULT means error accessing memory.

The earlier check returned EFAULT when ret is 0. Most probably, there was an assumption
that writing 0 bytes (by simple_write_to_buffer) means a fault with memory (or error accessing memory).