| Message ID | PS1PR04MB29345AB59076B370A4F99F75D6B39@PS1PR04MB2934.apcprd04.prod.outlook.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | arch:powerpc simple_write_to_buffer return check | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (44158b256b30415079588d0fcb1bccbdc2ccd009) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 1 warnings, 2 checks, 36 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
On Fri, Feb 5, 2021 at 5:17 AM Mayank Suman <mayanksuman@live.com> wrote: > > Signed-off-by: Mayank Suman <mayanksuman@live.com> commit messages aren't optional > --- > 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. > + 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. > /* > @@ -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 05/02/21 4:05 am, Oliver O'Halloran wrote: > On Fri, Feb 5, 2021 at 5:17 AM Mayank Suman <mayanksuman@live.com> wrote: >> >> Signed-off-by: Mayank Suman <mayanksuman@live.com> > > commit messages aren't optional Sorry. I will include the commit message in PATCH v2. > >> --- >> 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. > >> + 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 : > Signed-off-by: Mayank Suman <mayanksuman@live.com> > --- > 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 ? And why is it -EFAULT when ret is 0 ? EFAULT means error accessing memory. > > /* > @@ -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 */ >
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 : >> Signed-off-by: Mayank Suman <mayanksuman@live.com> >> --- >> 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).
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 */
Signed-off-by: Mayank Suman <mayanksuman@live.com> --- arch/powerpc/kernel/eeh.c | 8 ++++---- arch/powerpc/platforms/powernv/eeh-powernv.c | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-)