| Message ID | 202212231040562072342@zte.com.cn (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [linux-next] parisc: use strscpy() to instead of strncpy() | expand |
On 12/23/22 03:40, yang.yang29@zte.com.cn wrote: > From: Xu Panda <xu.panda@zte.com.cn> > > 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 <xu.panda@zte.com.cn> > Signed-off-by: Yang Yang <yang.yang29@zte.com> > --- > 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, yang.yang29@zte.com.cn wrote: > > From: Xu Panda <xu.panda@zte.com.cn> > > > > 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 <xu.panda@zte.com.cn> > > Signed-off-by: Yang Yang <yang.yang29@zte.com> > > --- > > 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, yang.yang29@zte.com.cn wrote: >>> From: Xu Panda <xu.panda@zte.com.cn> >>> >>> 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 <xu.panda@zte.com.cn> >>> Signed-off-by: Yang Yang <yang.yang29@zte.com> >>> --- >>> 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, yang.yang29@zte.com.cn wrote: > > > > From: Xu Panda <xu.panda@zte.com.cn> > > > > > > > > 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 <xu.panda@zte.com.cn> > > > > Signed-off-by: Yang Yang <yang.yang29@zte.com> > > > > --- > > > > 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, yang.yang29@zte.com.cn wrote: >>>>> From: Xu Panda <xu.panda@zte.com.cn> >>>>> >>>>> 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 <xu.panda@zte.com.cn> >>>>> Signed-off-by: Yang Yang <yang.yang29@zte.com> >>>>> --- >>>>> 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 >
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;