Messages in this thread | | | Date | Wed, 28 Dec 2022 08:28:55 +0100 | Subject | Re: [PATCH linux-next] parisc: use strscpy() to instead of strncpy() | From | Helge Deller <> |
| |
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 >
| |