| Message ID | c2cb8668-afc8-459a-9c91-9b0002fbeaa0@huawei.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [V2] Fix NULL dereference in super_by_fd | expand |
Dear lixiaokeng, Thank you for your patch. I found a few style issues. Am 14.12.22 um 04:17 schrieb lixiaokeng: > When we create 100 partitions(major is 259 not 254) in a raid device, Please add a space before the (. > mdadm may coredump: > > Core was generated by `/usr/sbin/mdadm --detail --export /dev/md1p3'. > Program terminated with signal SIGSEGV, Segmentation fault. > #0 __strlen_sse2 () > at ../sysdeps/x86_64/multiarch/strlen-vec.S:126 > 126 movdqu (%rax), %xmm4 > (gdb) bt > #0 __strlen_sse2 () > at ../sysdeps/x86_64/multiarch/strlen-vec.S:126 > #1 0x00007f1944659139 in __strcpy_chk ( > dest=dest@entry=0x55ea8d7c23ac "", src=0x0, > destlen=destlen@entry=32) at strcpy_chk.c:28 > #2 0x000055ea8d10b66d in strcpy (__src=<optimized out>, > __dest=0x55ea8d7c23ac "") > at /usr/include/bits/string_fortified.h:79 > #3 super_by_fd (fd=fd@entry=3, > subarrayp=subarrayp@entry=0x7ffe6a1dff08) at util.c:1289 > #4 0x000055ea8d11b3a6 in Detail ( > dev=0x7ffe6a1e2f22 "/dev/md1p3", c=0x7ffe6a1e1700) > at Detail.c:101 > #5 0x000055ea8d101e61 in misc_list (c=<optimized out>, > ss=<optimized out>, dump_directory=<optimized out>, > ident=<optimized out>, devlist=<optimized out>) > at mdadm.c:1959 > #6 main (argc=<optimized out>, argv=<optimized out>) > at mdadm.c:1629 Please do not wrap the pasted lines. > The direct cause is fd2devnm return NULL. Here add a check. … returning NULL, so add a check. > Signed-off-by:Lixiaokeng<lixiaokeng@huawei.com> > Signed-off-by:Wuguanghao<wuguanghao3@huawei.com> Please add a space after the colon, and before the <. Also, is Lixiaokeng your name, or could it be written Li Xiao Keng? Please use that format – `git config --global user.name "…"`. git commit --amend --author="Li Xiao Keng <lixiaokeng@huawei.com>" > --- As this is the second iteration (version 2) of the patch, it’s common, that you add a note, what the difference between this and the previous versions are. > mapfile.c | 4 ++++ > util.c | 7 ++++++- > 2 files changed, 10 insertions(+), 1 deletion(-) […] Kind regards, Paul
On Wed, 14 Dec 2022 11:17:41 +0800
lixiaokeng <lixiaokeng@huawei.com> wrote:
> strcpy(st->devnm, devnm);
Hi,
Please use strncpy or snprintf here.
Thanks,
Mariusz
On 2022/12/15 19:50, Mariusz Tkaczyk wrote: > On Wed, 14 Dec 2022 11:17:41 +0800 > lixiaokeng <lixiaokeng@huawei.com> wrote: > >> strcpy(st->devnm, devnm); > > Hi, > Please use strncpy or snprintf here. Thanks for your advice, but the length of devnm is not a defined value. I will keep it as the old codes. > > Thanks, > Mariusz > > . >
On 2022/12/14 15:55, Paul Menzel wrote: > Dear lixiaokeng, > > > Thank you for your patch. I found a few style issues. > > Am 14.12.22 um 04:17 schrieb lixiaokeng: >> When we create 100 partitions(major is 259 not 254) in a raid device, > > Please add a space before the (. > >> mdadm may coredump: >> >> Core was generated by `/usr/sbin/mdadm --detail --export /dev/md1p3'. >> Program terminated with signal SIGSEGV, Segmentation fault. >> #0 __strlen_sse2 () >> at ../sysdeps/x86_64/multiarch/strlen-vec.S:126 >> 126 movdqu (%rax), %xmm4 >> (gdb) bt >> #0 __strlen_sse2 () >> at ../sysdeps/x86_64/multiarch/strlen-vec.S:126 >> #1 0x00007f1944659139 in __strcpy_chk ( >> dest=dest@entry=0x55ea8d7c23ac "", src=0x0, >> destlen=destlen@entry=32) at strcpy_chk.c:28 >> #2 0x000055ea8d10b66d in strcpy (__src=<optimized out>, >> __dest=0x55ea8d7c23ac "") >> at /usr/include/bits/string_fortified.h:79 >> #3 super_by_fd (fd=fd@entry=3, >> subarrayp=subarrayp@entry=0x7ffe6a1dff08) at util.c:1289 >> #4 0x000055ea8d11b3a6 in Detail ( >> dev=0x7ffe6a1e2f22 "/dev/md1p3", c=0x7ffe6a1e1700) >> at Detail.c:101 >> #5 0x000055ea8d101e61 in misc_list (c=<optimized out>, >> ss=<optimized out>, dump_directory=<optimized out>, >> ident=<optimized out>, devlist=<optimized out>) >> at mdadm.c:1959 >> #6 main (argc=<optimized out>, argv=<optimized out>) >> at mdadm.c:1629 > > Please do not wrap the pasted lines. > >> The direct cause is fd2devnm return NULL. Here add a check. > > … returning NULL, so add a check. > >> Signed-off-by:Lixiaokeng<lixiaokeng@huawei.com> >> Signed-off-by:Wuguanghao<wuguanghao3@huawei.com> > > Please add a space after the colon, and before the <. Also, is Lixiaokeng your name, or could it be written Li Xiao Keng? Please use that format – `git config --global user.name "…"`. > > git commit --amend --author="Li Xiao Keng <lixiaokeng@huawei.com>" > >> --- > > As this is the second iteration (version 2) of the patch, it’s common, that you add a note, what the difference between this and the previous versions are. > >> mapfile.c | 4 ++++ >> util.c | 7 ++++++- >> 2 files changed, 10 insertions(+), 1 deletion(-) > > […] > > > Kind regards, > Thanks for your advices. I will change it and send it V3 again. > Paul > > .
On Mon, 19 Dec 2022 19:50:52 +0800 lixiaokeng <lixiaokeng@huawei.com> wrote: > On 2022/12/15 19:50, Mariusz Tkaczyk wrote: > > On Wed, 14 Dec 2022 11:17:41 +0800 > > lixiaokeng <lixiaokeng@huawei.com> wrote: > > > >> strcpy(st->devnm, devnm); > > > > Hi, > > Please use strncpy or snprintf here. > > Thanks for your advice, but the length of devnm is not > a defined value. I will keep it as the old codes. Supertype devnm is a array defined to be 32. https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/tree/mdadm.h#n1256 32 should be changed to MD_NAME_MAX - you can use this define. I traveled fd2devnm and I can see that at the end devid2devnm returns: static char devnm[32] https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/tree/lib.c#n123 For that reason usage of strcpy in this case seems to be safe, unless we change something deeper. My recommendation comes from general safe development rules- we know dest buffer size so we can esnure that it will be ended properly by '\0', whatever comes to write from fd2devnm(). Thanks, Mariusz
On 12/19/22 08:08, Mariusz Tkaczyk wrote: > On Mon, 19 Dec 2022 19:50:52 +0800 > lixiaokeng <lixiaokeng@huawei.com> wrote: > >> On 2022/12/15 19:50, Mariusz Tkaczyk wrote: >>> On Wed, 14 Dec 2022 11:17:41 +0800 >>> lixiaokeng <lixiaokeng@huawei.com> wrote: >>> >>>> strcpy(st->devnm, devnm); >>> >>> Hi, >>> Please use strncpy or snprintf here. >> >> Thanks for your advice, but the length of devnm is not >> a defined value. I will keep it as the old codes. > > Supertype devnm is a array defined to be 32. > https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/tree/mdadm.h#n1256 > > 32 should be changed to MD_NAME_MAX - you can use this define. > I traveled fd2devnm and I can see that at the end devid2devnm returns: > static char devnm[32] > https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/tree/lib.c#n123 > > For that reason usage of strcpy in this case seems to be safe, unless we change > something deeper. My recommendation comes from general safe development rules- > we know dest buffer size so we can esnure that it will be ended properly by > '\0', whatever comes to write from fd2devnm(). Totally agree here! Jes
On 2022/12/19 21:08, Mariusz Tkaczyk wrote: > On Mon, 19 Dec 2022 19:50:52 +0800 > lixiaokeng <lixiaokeng@huawei.com> wrote: > >> On 2022/12/15 19:50, Mariusz Tkaczyk wrote: >>> On Wed, 14 Dec 2022 11:17:41 +0800 >>> lixiaokeng <lixiaokeng@huawei.com> wrote: >>> >>>> strcpy(st->devnm, devnm); >>> >>> Hi, >>> Please use strncpy or snprintf here. >> >> Thanks for your advice, but the length of devnm is not >> a defined value. I will keep it as the old codes. > > Supertype devnm is a array defined to be 32. > https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/tree/mdadm.h#n1256 > > 32 should be changed to MD_NAME_MAX - you can use this define. > I traveled fd2devnm and I can see that at the end devid2devnm returns: > static char devnm[32] > https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/tree/lib.c#n123 > > For that reason usage of strcpy in this case seems to be safe, unless we change > something deeper. My recommendation comes from general safe development rules- > we know dest buffer size so we can esnure that it will be ended properly by > '\0', whatever comes to write from fd2devnm(). > > Thanks, > Mariusz > > Thanks, I understand it. I will modify the patch as V4. Regards, Li Xiaokeng > . >
diff --git a/mapfile.c b/mapfile.c index 8d7acb3..f72fe0d 100644 --- a/mapfile.c +++ b/mapfile.c @@ -292,6 +292,10 @@ struct map_ent *map_by_uuid(struct map_ent **map, int uuid[4]) struct map_ent *map_by_devnm(struct map_ent **map, char *devnm) { struct map_ent *mp; + + if (!devnm) + return NULL; + if (!*map) map_read(map); diff --git a/util.c b/util.c index 64dd409..3a84ee3 100644 --- a/util.c +++ b/util.c @@ -1241,6 +1241,11 @@ struct supertype *super_by_fd(int fd, char **subarrayp) int i; char *subarray = NULL; char container[32] = ""; + char *devnm = NULL; + + devnm = fd2devnm(fd); + if (!devnm) + return NULL; sra = sysfs_read(fd, NULL, GET_VERSION); @@ -1286,7 +1291,7 @@ struct supertype *super_by_fd(int fd, char **subarrayp) if (subarrayp) *subarrayp = subarray; strcpy(st->container_devnm, container); - strcpy(st->devnm, fd2devnm(fd)); + strcpy(st->devnm, devnm); } else free(subarray);
When we create 100 partitions(major is 259 not 254) in a raid device, mdadm may coredump: Core was generated by `/usr/sbin/mdadm --detail --export /dev/md1p3'. Program terminated with signal SIGSEGV, Segmentation fault. #0 __strlen_sse2 () at ../sysdeps/x86_64/multiarch/strlen-vec.S:126 126 movdqu (%rax), %xmm4 (gdb) bt #0 __strlen_sse2 () at ../sysdeps/x86_64/multiarch/strlen-vec.S:126 #1 0x00007f1944659139 in __strcpy_chk ( dest=dest@entry=0x55ea8d7c23ac "", src=0x0, destlen=destlen@entry=32) at strcpy_chk.c:28 #2 0x000055ea8d10b66d in strcpy (__src=<optimized out>, __dest=0x55ea8d7c23ac "") at /usr/include/bits/string_fortified.h:79 #3 super_by_fd (fd=fd@entry=3, subarrayp=subarrayp@entry=0x7ffe6a1dff08) at util.c:1289 #4 0x000055ea8d11b3a6 in Detail ( dev=0x7ffe6a1e2f22 "/dev/md1p3", c=0x7ffe6a1e1700) at Detail.c:101 #5 0x000055ea8d101e61 in misc_list (c=<optimized out>, ss=<optimized out>, dump_directory=<optimized out>, ident=<optimized out>, devlist=<optimized out>) at mdadm.c:1959 #6 main (argc=<optimized out>, argv=<optimized out>) at mdadm.c:1629 The direct cause is fd2devnm return NULL. Here add a check. Signed-off-by:Lixiaokeng<lixiaokeng@huawei.com> Signed-off-by:Wuguanghao<wuguanghao3@huawei.com> --- mapfile.c | 4 ++++ util.c | 7 ++++++- 2 files changed, 10 insertions(+), 1 deletion(-)