Skip to content

Commit a2f344a

Browse files
Kou Wenqimwilck
authored andcommitted
kpartx: fix crash and truncated device creation with long -p delimiter
When the -p delimiter is long enough to make the formatted partition name exceed PARTNAME_SIZE (128 bytes), three issues occur: 1. format_partname() fails but snprintf has already written a truncated name into the buffer. dm_find_part() returns 0 and the caller proceeds to dm_addmap() with the truncated name, creating a device that was never intended. 2. dm_find_part() returns early without setting *part_uuid. The uninitialized local variable part_uuid then gets passed to check_uuid() -> strchr(), causing a SIGSEGV. 3. The callers cannot distinguish between 'partition not found, create new' and 'name construction failed' since both return 0. Fix by: - Having dm_find_part() return DFP_ERR when format_partname() fails, with an error message printed by dm_find_part() itself - Defining an enum to represent the return values of dm_find_part(): enum { DFP_DEVICE_CREATE = DM_DEVICE_CREATE, DFP_DEVICE_RELOAD = DM_DEVICE_RELOAD, DFP_ERR = -1 }; This enum is placed in kpartx/devmapper.h, which now also includes <libdevmapper.h> directly to make the header self-contained. dm_find_part() uses these enum values explicitly in all return statements. - In the ADD/UPDATE loops (both main and container), assigning the return value directly to 'op' and checking for DFP_ERR to handle errors - In the DELETE loop, checking the return value against DFP_DEVICE_RELOAD to proceed with removal, skipping on error or not-found - Initializing part_uuid to NULL in all three partition loop bodies (ADD/UPDATE main loop, container partition loop, DELETE loop) so that the 'if (part_uuid && uuid)' guard correctly skips the UUID check when dm_find_part() returns early Reproduce steps (now also in the test-kpartx script): # Create test image dd if=/dev/zero of=/tmp/vhlg-test.img bs=1M count=10 parted /tmp/vhlg-test.img mklabel msdos parted /tmp/vhlg-test.img mkpart primary ext4 1MiB 5MiB # Reproduce kpartx -a -p $(python3 -c "print('A'*200)") /tmp/vhlg-test.img # Cleanup kpartx -d /tmp/vhlg-test.img rm -f /tmp/vhlg-test.img [mwilck: removed version history from commit message] Signed-off-by: Kou Wenqi <kouwenqi@kylinos.cn> Reviewed-by: Martin Wilck <mwilck@suse.com>
1 parent e8832b5 commit a2f344a

3 files changed

Lines changed: 40 additions & 24 deletions

File tree

kpartx/devmapper.c

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -644,18 +644,19 @@ int dm_find_part(const char *parent, const char *delim, int part,
644644
char dev_t[32];
645645

646646
if (!format_partname(name, namesiz, parent, delim, part)) {
647-
if (verbose)
648-
fprintf(stderr, "partname too small\n");
649-
return 0;
647+
fprintf(stderr, "partition name too long for partition %d\n", part);
648+
return DFP_ERR;
650649
}
651650

652651
r = dm_map_present(name, part_uuid);
653-
if (r == 1 || parent_uuid == NULL || *parent_uuid == '\0')
654-
return r;
652+
if (r == 1)
653+
return DFP_DEVICE_RELOAD;
654+
if (parent_uuid == NULL || *parent_uuid == '\0')
655+
return DFP_DEVICE_CREATE;
655656

656657
uuid = make_prefixed_uuid(part, parent_uuid);
657658
if (!uuid)
658-
return 0;
659+
return DFP_DEVICE_CREATE;
659660

660661
tmp = dm_find_uuid(uuid);
661662
if (tmp == NULL)
@@ -688,14 +689,14 @@ int dm_find_part(const char *parent, const char *delim, int part,
688689
if (r == 1) {
689690
free(tmp);
690691
*part_uuid = uuid;
691-
return 1;
692+
return DFP_DEVICE_RELOAD;
692693
}
693694
if (verbose)
694695
fprintf(stderr, "renaming %s->%s failed\n", tmp, name);
695696
out:
696697
free(uuid);
697698
free(tmp);
698-
return r;
699+
return DFP_DEVICE_CREATE;
699700
}
700701

701702
char *nondm_create_uuid(dev_t devt)

kpartx/devmapper.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
#ifndef KPARTX_DEVMAPPER_H_INCLUDED
22
#define KPARTX_DEVMAPPER_H_INCLUDED
33

4+
#include <libdevmapper.h>
5+
46
#ifdef DM_SUBSYSTEM_UDEV_FLAG0
57
#define MPATH_UDEV_RELOAD_FLAG DM_SUBSYSTEM_UDEV_FLAG0
68
#else
@@ -18,6 +20,13 @@ dev_t dm_get_first_dep(char *devname);
1820
char * dm_mapuuid(const char *mapname);
1921
int dm_devn (const char * mapname, unsigned int *major, unsigned int *minor);
2022
int dm_remove_partmaps (char * mapname, char *uuid, dev_t devt, int verbose);
23+
24+
enum {
25+
DFP_DEVICE_CREATE = DM_DEVICE_CREATE,
26+
DFP_DEVICE_RELOAD = DM_DEVICE_RELOAD,
27+
DFP_ERR = -1
28+
};
29+
2130
int dm_find_part(const char *parent, const char *delim, int part,
2231
const char *parent_uuid,
2332
char *name, size_t namesiz, char **part_uuid, int verbose);

kpartx/kpartx.c

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -437,7 +437,7 @@ main(int argc, char **argv){
437437
case UPDATE:
438438
/* ADD and UPDATE share the same code that adds new partitions. */
439439
for (j = 0, c = 0; j < n; j++) {
440-
char *part_uuid, *reason;
440+
char *part_uuid = NULL, *reason;
441441

442442
if (slices[j].size == 0)
443443
continue;
@@ -454,10 +454,13 @@ main(int argc, char **argv){
454454
exit(1);
455455
}
456456

457-
op = (dm_find_part(mapname, delim, j + 1, uuid,
458-
partname, sizeof(partname),
459-
&part_uuid, verbose) ?
460-
DM_DEVICE_RELOAD : DM_DEVICE_CREATE);
457+
op = dm_find_part(mapname, delim, j + 1, uuid,
458+
partname, sizeof(partname),
459+
&part_uuid, verbose);
460+
if (op == DFP_ERR) {
461+
r++;
462+
continue;
463+
}
461464

462465
if (part_uuid && uuid) {
463466
if (check_uuid(uuid, part_uuid, &reason) != 0) {
@@ -500,7 +503,7 @@ main(int argc, char **argv){
500503
d = c;
501504
while (c) {
502505
for (j = 0; j < n; j++) {
503-
char *part_uuid, *reason;
506+
char *part_uuid = NULL, *reason;
504507
int k = slices[j].container - 1;
505508

506509
if (slices[j].size == 0)
@@ -526,11 +529,14 @@ main(int argc, char **argv){
526529
exit(1);
527530
}
528531

529-
op = (dm_find_part(mapname, delim, j + 1, uuid,
530-
partname,
531-
sizeof(partname),
532-
&part_uuid, verbose) ?
533-
DM_DEVICE_RELOAD : DM_DEVICE_CREATE);
532+
op = dm_find_part(mapname, delim,
533+
j + 1, uuid, partname,
534+
sizeof(partname),
535+
&part_uuid, verbose);
536+
if (op == DFP_ERR) {
537+
r++;
538+
continue;
539+
}
534540

535541
if (part_uuid && uuid) {
536542
if (check_uuid(uuid, part_uuid, &reason) != 0) {
@@ -570,11 +576,11 @@ main(int argc, char **argv){
570576
}
571577

572578
for (j = MAXSLICES-1; j >= 0; j--) {
573-
char *part_uuid, *reason;
574-
if (slices[j].size ||
575-
!dm_find_part(mapname, delim, j + 1, uuid,
576-
partname, sizeof(partname),
577-
&part_uuid, verbose))
579+
char *part_uuid = NULL, *reason;
580+
int res = dm_find_part(mapname, delim, j + 1, uuid,
581+
partname, sizeof(partname),
582+
&part_uuid, verbose);
583+
if (slices[j].size || res != DFP_DEVICE_RELOAD)
578584
continue;
579585

580586
if (part_uuid && uuid) {

0 commit comments

Comments
 (0)