| Message ID | 20210621064938.2021419-1-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] powerpc/prom_init: Convert prom_strcpy() into prom_strscpy_pad() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (7f030e9d57b8ff6025bde4162f42378e6081126a) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 44 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Hi > -static char __init *prom_strcpy(char *dest, const char *src) > +static ssize_t __init prom_strscpy_pad(char *dest, const char *src, size_t n) > { > - char *tmp = dest; > + ssize_t rc; > + size_t i; > > - while ((*dest++ = *src++) != '\0') > - /* nothing */; > - return tmp; > + if (n == 0 || n > INT_MAX) > + return -E2BIG; > + > + // Copy up to n bytes > + for (i = 0; i < n && src[i] != '\0'; i++) > + dest[i] = src[i]; > + > + rc = i; > + > + // If we copied all n then we have run out of space for the nul > + if (rc == n) { > + // Rewind by one character to ensure nul termination > + i--; > + rc = -E2BIG; > + } > + > + for (; i < n; i++) > + dest[i] = '\0'; > + > + return rc; > } > This implementation seems good to me. I copied it into a new C file and added the following: int main() { char longstr[255]="abcdefghijklmnopqrstuvwxyz"; char shortstr[5]; assert(prom_strscpy_pad(longstr, "", 0) == -E2BIG); assert(prom_strscpy_pad(longstr, "hello", 255) == 5); assert(prom_strscpy_pad(shortstr, "hello", 5) == -E2BIG); assert(memcmp(shortstr, "hell", 5) == 0); assert(memcmp(longstr, "hello\0\0\0\0\0\0\0\0\0", 6) == 0); return 0; } All the assertions pass. I believe this covers all the conditions from the strscpy_pad docstring. Reviewed-by: Daniel Axtens <dja@axtens.net> Kind regards, Daniel > static int __init prom_strncmp(const char *cs, const char *ct, size_t count) > @@ -2701,7 +2719,7 @@ static void __init flatten_device_tree(void) > > /* Add "phandle" in there, we'll need it */ > namep = make_room(&mem_start, &mem_end, 16, 1); > - prom_strcpy(namep, "phandle"); > + prom_strscpy_pad(namep, "phandle", sizeof("phandle")); > mem_start = (unsigned long)namep + prom_strlen(namep) + 1; > > /* Build string array */ > -- > 2.25.1
Daniel Axtens <dja@axtens.net> writes: > Hi > >> -static char __init *prom_strcpy(char *dest, const char *src) >> +static ssize_t __init prom_strscpy_pad(char *dest, const char *src, size_t n) >> { >> - char *tmp = dest; >> + ssize_t rc; >> + size_t i; >> >> - while ((*dest++ = *src++) != '\0') >> - /* nothing */; >> - return tmp; >> + if (n == 0 || n > INT_MAX) >> + return -E2BIG; >> + >> + // Copy up to n bytes >> + for (i = 0; i < n && src[i] != '\0'; i++) >> + dest[i] = src[i]; >> + >> + rc = i; >> + >> + // If we copied all n then we have run out of space for the nul >> + if (rc == n) { >> + // Rewind by one character to ensure nul termination >> + i--; >> + rc = -E2BIG; >> + } >> + >> + for (; i < n; i++) >> + dest[i] = '\0'; >> + >> + return rc; >> } >> > > This implementation seems good to me. > > I copied it into a new C file and added the following: > > int main() { > char longstr[255]="abcdefghijklmnopqrstuvwxyz"; > char shortstr[5]; > assert(prom_strscpy_pad(longstr, "", 0) == -E2BIG); > assert(prom_strscpy_pad(longstr, "hello", 255) == 5); > assert(prom_strscpy_pad(shortstr, "hello", 5) == -E2BIG); > assert(memcmp(shortstr, "hell", 5) == 0); > assert(memcmp(longstr, "hello\0\0\0\0\0\0\0\0\0", 6) == 0); > return 0; > } > > All the assertions pass. I believe this covers all the conditions from > the strscpy_pad docstring. > > Reviewed-by: Daniel Axtens <dja@axtens.net> Thanks. I'll also drop the explicit nul termination in patch 2, which is a leftover from when I was using strncpy(). cheers
On 6/21/21 9:11 PM, Michael Ellerman wrote: > Daniel Axtens <dja@axtens.net> writes: >> Hi >> >>> -static char __init *prom_strcpy(char *dest, const char *src) >>> +static ssize_t __init prom_strscpy_pad(char *dest, const char *src, size_t n) >>> { >>> - char *tmp = dest; >>> + ssize_t rc; >>> + size_t i; >>> >>> - while ((*dest++ = *src++) != '\0') >>> - /* nothing */; >>> - return tmp; >>> + if (n == 0 || n > INT_MAX) >>> + return -E2BIG; >>> + >>> + // Copy up to n bytes >>> + for (i = 0; i < n && src[i] != '\0'; i++) >>> + dest[i] = src[i]; >>> + >>> + rc = i; >>> + >>> + // If we copied all n then we have run out of space for the nul >>> + if (rc == n) { >>> + // Rewind by one character to ensure nul termination >>> + i--; >>> + rc = -E2BIG; >>> + } >>> + >>> + for (; i < n; i++) >>> + dest[i] = '\0'; >>> + >>> + return rc; >>> } >>> >> >> This implementation seems good to me. >> >> I copied it into a new C file and added the following: >> >> int main() { >> char longstr[255]="abcdefghijklmnopqrstuvwxyz"; >> char shortstr[5]; >> assert(prom_strscpy_pad(longstr, "", 0) == -E2BIG); >> assert(prom_strscpy_pad(longstr, "hello", 255) == 5); >> assert(prom_strscpy_pad(shortstr, "hello", 5) == -E2BIG); >> assert(memcmp(shortstr, "hell", 5) == 0); >> assert(memcmp(longstr, "hello\0\0\0\0\0\0\0\0\0", 6) == 0); >> return 0; >> } >> >> All the assertions pass. I believe this covers all the conditions from >> the strscpy_pad docstring. >> >> Reviewed-by: Daniel Axtens <dja@axtens.net> > > Thanks. > > I'll also drop the explicit nul termination in patch 2, which is a > leftover from when I was using strncpy(). I guess you can ignore my other email questioning this. -Tyrel > > cheers >
On Mon, 21 Jun 2021 16:49:37 +1000, Michael Ellerman wrote: > In a subsequent patch we'd like to have something like a strscpy_pad() > implementation usable in prom_init.c. > > Currently we have a strcpy() implementation with only one caller, so > convert it into strscpy_pad() and update the caller. Applied to powerpc/next. [1/2] powerpc/prom_init: Convert prom_strcpy() into prom_strscpy_pad() https://git.kernel.org/powerpc/c/f47d5a4fc254e62ea5af5cbb2fc3e68901def434 [2/2] powerpc/prom_init: Pass linux_banner to firmware via option vector 7 https://git.kernel.org/powerpc/c/ffaacd97fd37b9f4e825d8107f5cba5470458f0e cheers
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c index 523b31685c4c..c18d55f8b951 100644 --- a/arch/powerpc/kernel/prom_init.c +++ b/arch/powerpc/kernel/prom_init.c @@ -242,13 +242,31 @@ static int __init prom_strcmp(const char *cs, const char *ct) return 0; } -static char __init *prom_strcpy(char *dest, const char *src) +static ssize_t __init prom_strscpy_pad(char *dest, const char *src, size_t n) { - char *tmp = dest; + ssize_t rc; + size_t i; - while ((*dest++ = *src++) != '\0') - /* nothing */; - return tmp; + if (n == 0 || n > INT_MAX) + return -E2BIG; + + // Copy up to n bytes + for (i = 0; i < n && src[i] != '\0'; i++) + dest[i] = src[i]; + + rc = i; + + // If we copied all n then we have run out of space for the nul + if (rc == n) { + // Rewind by one character to ensure nul termination + i--; + rc = -E2BIG; + } + + for (; i < n; i++) + dest[i] = '\0'; + + return rc; } static int __init prom_strncmp(const char *cs, const char *ct, size_t count) @@ -2701,7 +2719,7 @@ static void __init flatten_device_tree(void) /* Add "phandle" in there, we'll need it */ namep = make_room(&mem_start, &mem_end, 16, 1); - prom_strcpy(namep, "phandle"); + prom_strscpy_pad(namep, "phandle", sizeof("phandle")); mem_start = (unsigned long)namep + prom_strlen(namep) + 1; /* Build string array */
In a subsequent patch we'd like to have something like a strscpy_pad() implementation usable in prom_init.c. Currently we have a strcpy() implementation with only one caller, so convert it into strscpy_pad() and update the caller. Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> --- arch/powerpc/kernel/prom_init.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)