From: Xu Panda <[email protected]>
The implementation of strscpy() is more robust and safer.
That's now the recommended way to copy NUL-terminated strings.
Signed-off-by: Xu Panda <[email protected]>
Signed-off-by: Yang Yang <[email protected]>
---
drivers/parisc/pdc_stable.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/parisc/pdc_stable.c b/drivers/parisc/pdc_stable.c
index d6af5726ddf3..403bca0021c5 100644
--- a/drivers/parisc/pdc_stable.c
+++ b/drivers/parisc/pdc_stable.c
@@ -274,8 +274,7 @@ pdcspath_hwpath_write(struct pdcspath_entry *entry, const char *buf, size_t coun
/* We'll use a local copy of buf */
count = min_t(size_t, count, sizeof(in)-1);
- strncpy(in, buf, count);
- in[count] = '\0';
+ strscpy(in, buf, count + 1);
/* Let's clean up the target. 0xff is a blank pattern */
memset(&hwpath, 0xff, sizeof(hwpath));
@@ -388,8 +387,7 @@ pdcspath_layer_write(struct pdcspath_entry *entry, const char *buf, size_t count
/* We'll use a local copy of buf */
count = min_t(size_t, count, sizeof(in)-1);
- strncpy(in, buf, count);
- in[count] = '\0';
+ strscpy(in, buf, count + 1);
/* Let's clean up the target. 0 is a blank pattern */
memset(&layers, 0, sizeof(layers));
@@ -756,8 +754,7 @@ static ssize_t pdcs_auto_write(struct kobject *kobj,
/* We'll use a local copy of buf */
count = min_t(size_t, count, sizeof(in)-1);
- strncpy(in, buf, count);
- in[count] = '\0';
+ strscpy(in, buf, count + 1);
/* Current flags are stored in primary boot path entry */
pathentry = &pdcspath_entry_primary;
--
2.15.2
On 12/23/22 03:40, [email protected] wrote:
> From: Xu Panda <[email protected]>
>
> The implementation of strscpy() is more robust and safer.
> That's now the recommended way to copy NUL-terminated strings.
Thanks for your patch, but....
> Signed-off-by: Xu Panda <[email protected]>
> Signed-off-by: Yang Yang <[email protected]>
> ---
> drivers/parisc/pdc_stable.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/parisc/pdc_stable.c b/drivers/parisc/pdc_stable.c
> index d6af5726ddf3..403bca0021c5 100644
> --- a/drivers/parisc/pdc_stable.c
> +++ b/drivers/parisc/pdc_stable.c
> @@ -274,8 +274,7 @@ pdcspath_hwpath_write(struct pdcspath_entry *entry, const char *buf, size_t coun
>
> /* We'll use a local copy of buf */
> count = min_t(size_t, count, sizeof(in)-1);
> - strncpy(in, buf, count);
> - in[count] = '\0';
> + strscpy(in, buf, count + 1);
could you resend it somewhat simplified, e.g.
strscpy(in, buf, sizeof(in));
>
> /* Let's clean up the target. 0xff is a blank pattern */
> memset(&hwpath, 0xff, sizeof(hwpath));
> @@ -388,8 +387,7 @@ pdcspath_layer_write(struct pdcspath_entry *entry, const char *buf, size_t count
>
> /* We'll use a local copy of buf */
> count = min_t(size_t, count, sizeof(in)-1);
> - strncpy(in, buf, count);
> - in[count] = '\0';
> + strscpy(in, buf, count + 1);
same here
>
> /* Let's clean up the target. 0 is a blank pattern */
> memset(&layers, 0, sizeof(layers));
> @@ -756,8 +754,7 @@ static ssize_t pdcs_auto_write(struct kobject *kobj,
>
> /* We'll use a local copy of buf */
> count = min_t(size_t, count, sizeof(in)-1);
> - strncpy(in, buf, count);
> - in[count] = '\0';
> + strscpy(in, buf, count + 1);
and here
>
> /* Current flags are stored in primary boot path entry */
> pathentry = &pdcspath_entry_primary;
On Fri, 2022-12-23 at 08:55 +0100, Helge Deller wrote:
> On 12/23/22 03:40, [email protected] wrote:
> > From: Xu Panda <[email protected]>
> >
> > The implementation of strscpy() is more robust and safer.
> > That's now the recommended way to copy NUL-terminated strings.
>
> Thanks for your patch, but....
>
> > Signed-off-by: Xu Panda <[email protected]>
> > Signed-off-by: Yang Yang <[email protected]>
> > ---
> > drivers/parisc/pdc_stable.c | 9 +++------
> > 1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/parisc/pdc_stable.c
> > b/drivers/parisc/pdc_stable.c
> > index d6af5726ddf3..403bca0021c5 100644
> > --- a/drivers/parisc/pdc_stable.c
> > +++ b/drivers/parisc/pdc_stable.c
> > @@ -274,8 +274,7 @@ pdcspath_hwpath_write(struct pdcspath_entry
> > *entry, const char *buf, size_t coun
> >
> > /* We'll use a local copy of buf */
> > count = min_t(size_t, count, sizeof(in)-1);
> > - strncpy(in, buf, count);
> > - in[count] = '\0';
> > + strscpy(in, buf, count + 1);
>
> could you resend it somewhat simplified, e.g.
> strscpy(in, buf, sizeof(in));
I don't think you can: count is the size of buf, if that's < sizeof(in)
you've introduced a write beyond end of buffer. In fact sysfs tends to
pass pages as buffers, so there's no actual problem, but if that ever
changed ...
James
Hi James,
On 12/27/22 13:38, James Bottomley wrote:
> On Fri, 2022-12-23 at 08:55 +0100, Helge Deller wrote:
>> On 12/23/22 03:40, [email protected] wrote:
>>> From: Xu Panda <[email protected]>
>>>
>>> The implementation of strscpy() is more robust and safer.
>>> That's now the recommended way to copy NUL-terminated strings.
>>
>> Thanks for your patch, but....
>>
>>> Signed-off-by: Xu Panda <[email protected]>
>>> Signed-off-by: Yang Yang <[email protected]>
>>> ---
>>> drivers/parisc/pdc_stable.c | 9 +++------
>>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/parisc/pdc_stable.c
>>> b/drivers/parisc/pdc_stable.c
>>> index d6af5726ddf3..403bca0021c5 100644
>>> --- a/drivers/parisc/pdc_stable.c
>>> +++ b/drivers/parisc/pdc_stable.c
>>> @@ -274,8 +274,7 @@ pdcspath_hwpath_write(struct pdcspath_entry
>>> *entry, const char *buf, size_t coun
>>>
>>> /* We'll use a local copy of buf */
>>> count = min_t(size_t, count, sizeof(in)-1);
>>> - strncpy(in, buf, count);
>>> - in[count] = '\0';
>>> + strscpy(in, buf, count + 1);
>>
>> could you resend it somewhat simplified, e.g.
>> strscpy(in, buf, sizeof(in));
>
> I don't think you can: count is the size of buf, if that's < sizeof(in)
> you've introduced a write beyond end of buffer. In fact sysfs tends to
> pass pages as buffers, so there's no actual problem, but if that ever
> changed ...
Huh?... he doesn't change "count", so what's wrong with the latest patch?
Helge
On Tue, 2022-12-27 at 22:38 +0100, Helge Deller wrote:
> Hi James,
>
> On 12/27/22 13:38, James Bottomley wrote:
> > On Fri, 2022-12-23 at 08:55 +0100, Helge Deller wrote:
> > > On 12/23/22 03:40, [email protected] wrote:
> > > > From: Xu Panda <[email protected]>
> > > >
> > > > The implementation of strscpy() is more robust and safer.
> > > > That's now the recommended way to copy NUL-terminated strings.
> > >
> > > Thanks for your patch, but....
> > >
> > > > Signed-off-by: Xu Panda <[email protected]>
> > > > Signed-off-by: Yang Yang <[email protected]>
> > > > ---
> > > > drivers/parisc/pdc_stable.c | 9 +++------
> > > > 1 file changed, 3 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/parisc/pdc_stable.c
> > > > b/drivers/parisc/pdc_stable.c
> > > > index d6af5726ddf3..403bca0021c5 100644
> > > > --- a/drivers/parisc/pdc_stable.c
> > > > +++ b/drivers/parisc/pdc_stable.c
> > > > @@ -274,8 +274,7 @@ pdcspath_hwpath_write(struct pdcspath_entry
> > > > *entry, const char *buf, size_t coun
> > > >
> > > > /* We'll use a local copy of buf */
> > > > count = min_t(size_t, count, sizeof(in)-1);
> > > > - strncpy(in, buf, count);
> > > > - in[count] = '\0';
> > > > + strscpy(in, buf, count + 1);
> > >
> > > could you resend it somewhat simplified, e.g.
> > > strscpy(in, buf, sizeof(in));
> >
> > I don't think you can: count is the size of buf, if that's <
> > sizeof(in) you've introduced a write beyond end of buffer. In fact
> > sysfs tends to pass pages as buffers, so there's no actual problem,
> > but if that ever changed ...
>
> Huh?... he doesn't change "count", so what's wrong with the latest
> patch?
the array buf[] is actually buf[count], so if count < 64 then
sizeof(buf) < sizeof(in) and you're copying whatever is after buf on
the stack or wherever it comes from. The amount you copy into in[]
truly has to be the smaller of count and sizeof(in). These are file
operations, so you shouldn't rely on buf[] being null terminated
(kernfs ensures it is, but it's a dangerous thing to rely on in the
face of someone trying to exploit a stack smashing attack).
James
> the array buf[] is actually buf[count], so if count < 64 then
> sizeof(buf) < sizeof(in) and you're copying whatever is after buf on
> the stack or wherever it comes from. The amount you copy into in[]
> truly has to be the smaller of count and sizeof(in). These are file
> operations, so you shouldn't rely on buf[] being null terminated
> (kernfs ensures it is, but it's a dangerous thing to rely on in the
> face of someone trying to exploit a stack smashing attack).
Should we send patchv3 which is back to v1, or we directly use
patchv1 to continue the reviewing?
Thanks!
On 12/27/22 23:43, James Bottomley wrote:
> On Tue, 2022-12-27 at 22:38 +0100, Helge Deller wrote:
>> Hi James,
>>
>> On 12/27/22 13:38, James Bottomley wrote:
>>> On Fri, 2022-12-23 at 08:55 +0100, Helge Deller wrote:
>>>> On 12/23/22 03:40, [email protected] wrote:
>>>>> From: Xu Panda <[email protected]>
>>>>>
>>>>> The implementation of strscpy() is more robust and safer.
>>>>> That's now the recommended way to copy NUL-terminated strings.
>>>>
>>>> Thanks for your patch, but....
>>>>
>>>>> Signed-off-by: Xu Panda <[email protected]>
>>>>> Signed-off-by: Yang Yang <[email protected]>
>>>>> ---
>>>>> drivers/parisc/pdc_stable.c | 9 +++------
>>>>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/parisc/pdc_stable.c
>>>>> b/drivers/parisc/pdc_stable.c
>>>>> index d6af5726ddf3..403bca0021c5 100644
>>>>> --- a/drivers/parisc/pdc_stable.c
>>>>> +++ b/drivers/parisc/pdc_stable.c
>>>>> @@ -274,8 +274,7 @@ pdcspath_hwpath_write(struct pdcspath_entry
>>>>> *entry, const char *buf, size_t coun
>>>>>
>>>>> /* We'll use a local copy of buf */
>>>>> count = min_t(size_t, count, sizeof(in)-1);
>>>>> - strncpy(in, buf, count);
>>>>> - in[count] = '\0';
>>>>> + strscpy(in, buf, count + 1);
>>>>
>>>> could you resend it somewhat simplified, e.g.
>>>> strscpy(in, buf, sizeof(in));
>>>
>>> I don't think you can: count is the size of buf, if that's <
>>> sizeof(in) you've introduced a write beyond end of buffer. In fact
>>> sysfs tends to pass pages as buffers, so there's no actual problem,
>>> but if that ever changed ...
>>
>> Huh?... he doesn't change "count", so what's wrong with the latest
>> patch?
>
> the array buf[] is actually buf[count], so if count < 64 then
> sizeof(buf) < sizeof(in) and you're copying whatever is after buf on
> the stack or wherever it comes from. The amount you copy into in[]
> truly has to be the smaller of count and sizeof(in). These are file
> operations, so you shouldn't rely on buf[] being null terminated
Ok, the main point I missed was that buf[] might not be null terminated.
Thanks for the explanation.
Yang & Xu, no need to resend the patch. I'll take your v1 version.
Thanks!
Helge
> (kernfs ensures it is, but it's a dangerous thing to rely on in the
> face of someone trying to exploit a stack smashing attack).
>
> James
>