| Message ID | 1336691159-31764-1-git-send-email-snijsure@grid-net.com |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2012-05-10 at 16:05 -0700, Subodh Nijsure wrote: > + * Verify security.selinux extendend attribute for given path > + */ > +void xattr_check(char *path, char *xattr_value) > +{ > + int ret; > + char buf[255]; > + int attrLen; > + /* Check if path actually exists */ > + if (access(path, F_OK) != 0) > + return; > + if (xattr_value == NULL) > + return; > + attrLen = strlen(xattr_value) + 1; > + v("retrieve extended attribute for %s", path); > + getxattr(path, "security.selinux", buf, attrLen); > + CHECK(ret == 0); > + ret = strncmp(buf, xattr_value, attrLen); > + if (ret != 0) { > + printf("ret != 0 PATH %s\n", path); > + printf("Expected value %s and retrived val %s\n", > + xattr_value, buf); > + CHECK(ret == 0); > + } else { > + CHECK(ret == 0); > + } If the file-system does not support xattrs and returns ENOTSUPP, you should not fail. Instead, you should set a "xattrs_supported" variable, set it to zero, and have "fi (!xattrs_supported) return" check at the top level of this function. Or something like this which handles the situation gracefully.
Hi, On Thu, 2012-05-10 at 16:05 -0700, Subodh Nijsure wrote: > Changes since v1: > Randomize value of extended attribute. > Update dir_entry structure to keep track of extended attribute and > check it during verification phase, as suggested during v1 review. > Now compiling and running integck requires libattr. > > Signed-off-by: Subodh Nijsure <snijsure@grid-net.com> Please, fix these compiler warnings: gcc -Wall -g -O2 -I../../../include -I../../../ubi-utils//include -c ../../../ubi-utils//libubi.c -o libubi.o ar cr libubi.a libubi.o gcc -Wall -g -O2 -I../../../include -I../../../ubi-utils//include integck.c libubi.a -o integck integck.c: In function ‘xattr_check’: integck.c:379:2: warning: implicit declaration of function ‘getxattr’ [-Wimplicit-function-declaration] integck.c: In function ‘assign_xattr’: integck.c:410:2: warning: implicit declaration of function ‘setxattr’ [-Wimplicit-function-declaration] integck.c: In function ‘xattr_check’: integck.c:380:2: warning: ‘ret’ may be used uninitialized in this function [-Wuninitialized] Below are some comments - some of them apply to more places, so do not forget to check other places in your patch for the same things. > diff --git a/tests/fs-tests/integrity/Makefile b/tests/fs-tests/integrity/Makefile > index 4d6fc7d..b814f77 100644 > --- a/tests/fs-tests/integrity/Makefile > +++ b/tests/fs-tests/integrity/Makefile > @@ -25,7 +25,7 @@ $(TARGETS): libubi.a > # Disable optimizations to make it possible to use gdb comfortably > # Use -rdynamic to have stack backtraces > debug: libubi.a > - gcc $(CFLAGS) -O0 -D INTEGCK_DEBUG -rdynamic integck.c libubi.a -o integck > + gcc $(CFLAGS) -O0 -D INTEGCK_DEBUG -rdynamic integck.c libubi.a -o integck -lattr > > clean: > rm -f *.o $(TARGETS) libubi.a > diff --git a/tests/fs-tests/integrity/integck.c b/tests/fs-tests/integrity/integck.c > index 30322cd..7bd73bf 100644 > --- a/tests/fs-tests/integrity/integck.c > +++ b/tests/fs-tests/integrity/integck.c > @@ -91,6 +91,7 @@ > normsg(fmt " (line %d)", ##__VA_ARGS__, __LINE__); \ > } while(0) > > +static int xattr_count; > /* The variables below are set by command line arguments */ > static struct { > long repeat_cnt; > @@ -198,6 +199,7 @@ struct dir_entry_info /* Each entry in a directory has one of these */ > struct dir_entry_info *next_link; /* List of hard links for same file */ > struct dir_entry_info *prev_link; /* List of hard links for same file */ > char *name; > + char *xattr_value; /* Extended attribute on this directory */ > struct dir_info *parent; /* Parent directory */ > union { > struct file_info *file; > @@ -360,6 +362,60 @@ static char *cat_paths(const char *a, const char *b) > } > > /* > + * Verify security.selinux extendend attribute for given path > + */ > +void xattr_check(char *path, char *xattr_value) > +{ > + int ret; > + char buf[255]; Introduce a macro for max xattr length instead of a magic number. > + int attrLen; > + /* Check if path actually exists */ > + if (access(path, F_OK) != 0) > + return; > + if (xattr_value == NULL) > + return; It is faster to first check xattr_value, then make the "access" syscall. Also, please the style this file uses - "if (!xattr)". > + attrLen = strlen(xattr_value) + 1; > + v("retrieve extended attribute for %s", path); > + getxattr(path, "security.selinux", buf, attrLen); > + CHECK(ret == 0); > + ret = strncmp(buf, xattr_value, attrLen); > + if (ret != 0) { > + printf("ret != 0 PATH %s\n", path); > + printf("Expected value %s and retrived val %s\n", > + xattr_value, buf); Use errmsg(). > + CHECK(ret == 0); > + } else { > + CHECK(ret == 0); > + } > +} > + > +/* > + * Assign security.selinux extendend attribute for this path > + */ > +char *assign_xattr(const char *path) > +{ > + int ret; > + char value[255], *xattr_value; > + int attrLen; > + > + sprintf(value, "root:object_r:bin_t%d", xattr_count); > + xattr_count++; > + xattr_value = (char *)malloc(strlen(value)+1); sizefo(value) instead of strlen. Remove the cast after malloc. > + if (xattr_value == NULL) > + return NULL; > + strcpy(xattr_value, value); > + attrLen = strlen(xattr_value) + 1; > + v("assign extended attribute to %s", path); > + /* printf("%s assigned %s\n", path, xattr_value); */ Kill this comment. > + ret = setxattr(path, "security.selinux", xattr_value, attrLen, 0x0); > + if (ret == 0) > + return xattr_value; > + else { > + free(xattr_value); > + return NULL; > + } > +} Please, do not assign an xattr to all files and directories - better a give it a 20% chance or something like this. This is better for test coverage I thing. Add something like: if (random_no(5) != 0) return at the beginning of this function. > diff --git a/tests/ubi-tests/Makefile b/tests/ubi-tests/Makefile > index 2c47a9f..ba8e7e0 100644 > --- a/tests/ubi-tests/Makefile > +++ b/tests/ubi-tests/Makefile > @@ -8,7 +8,8 @@ LIBS = libubi > TARGETS=io_update volrefcnt integ io_paral io_read io_basic \ > mkvol_basic mkvol_bad mkvol_paral rsvol > > -CFLAGS += -I$(LIBUBI_HEADER_PATH) -I $(KERNELHDR) -lpthread > +CFLAGS += -I$(LIBUBI_HEADER_PATH) -I$(KERNELHDR) -lpthread > +LDFLAGS += -lpthread This change seems to have nothing to do with integck - please, kill this chunk.
diff --git a/tests/fs-tests/integrity/Makefile b/tests/fs-tests/integrity/Makefile index 4d6fc7d..b814f77 100644 --- a/tests/fs-tests/integrity/Makefile +++ b/tests/fs-tests/integrity/Makefile @@ -25,7 +25,7 @@ $(TARGETS): libubi.a # Disable optimizations to make it possible to use gdb comfortably # Use -rdynamic to have stack backtraces debug: libubi.a - gcc $(CFLAGS) -O0 -D INTEGCK_DEBUG -rdynamic integck.c libubi.a -o integck + gcc $(CFLAGS) -O0 -D INTEGCK_DEBUG -rdynamic integck.c libubi.a -o integck -lattr clean: rm -f *.o $(TARGETS) libubi.a diff --git a/tests/fs-tests/integrity/integck.c b/tests/fs-tests/integrity/integck.c index 30322cd..7bd73bf 100644 --- a/tests/fs-tests/integrity/integck.c +++ b/tests/fs-tests/integrity/integck.c @@ -91,6 +91,7 @@ normsg(fmt " (line %d)", ##__VA_ARGS__, __LINE__); \ } while(0) +static int xattr_count; /* The variables below are set by command line arguments */ static struct { long repeat_cnt; @@ -198,6 +199,7 @@ struct dir_entry_info /* Each entry in a directory has one of these */ struct dir_entry_info *next_link; /* List of hard links for same file */ struct dir_entry_info *prev_link; /* List of hard links for same file */ char *name; + char *xattr_value; /* Extended attribute on this directory */ struct dir_info *parent; /* Parent directory */ union { struct file_info *file; @@ -360,6 +362,60 @@ static char *cat_paths(const char *a, const char *b) } /* + * Verify security.selinux extendend attribute for given path + */ +void xattr_check(char *path, char *xattr_value) +{ + int ret; + char buf[255]; + int attrLen; + /* Check if path actually exists */ + if (access(path, F_OK) != 0) + return; + if (xattr_value == NULL) + return; + attrLen = strlen(xattr_value) + 1; + v("retrieve extended attribute for %s", path); + getxattr(path, "security.selinux", buf, attrLen); + CHECK(ret == 0); + ret = strncmp(buf, xattr_value, attrLen); + if (ret != 0) { + printf("ret != 0 PATH %s\n", path); + printf("Expected value %s and retrived val %s\n", + xattr_value, buf); + CHECK(ret == 0); + } else { + CHECK(ret == 0); + } +} + +/* + * Assign security.selinux extendend attribute for this path + */ +char *assign_xattr(const char *path) +{ + int ret; + char value[255], *xattr_value; + int attrLen; + + sprintf(value, "root:object_r:bin_t%d", xattr_count); + xattr_count++; + xattr_value = (char *)malloc(strlen(value)+1); + if (xattr_value == NULL) + return NULL; + strcpy(xattr_value, value); + attrLen = strlen(xattr_value) + 1; + v("assign extended attribute to %s", path); + /* printf("%s assigned %s\n", path, xattr_value); */ + ret = setxattr(path, "security.selinux", xattr_value, attrLen, 0x0); + if (ret == 0) + return xattr_value; + else { + free(xattr_value); + return NULL; + } +} +/* * Get the free space for the tested file system. */ static void get_fs_space(uint64_t *total, uint64_t *free) @@ -450,13 +506,17 @@ static void free_writes_info(struct file_info *file) } static void *add_dir_entry(struct dir_info *parent, char type, const char *name, - void *target) + const char *xattr_value, void *target) { struct dir_entry_info *entry; entry = zalloc(sizeof(struct dir_entry_info)); entry->type = type; entry->name = dup_string(name); + if (xattr_value != NULL) + entry->xattr_value = dup_string(xattr_value); + else + entry->xattr_value = NULL; entry->parent = parent; entry->next = parent->first; @@ -540,6 +600,7 @@ static void remove_dir_entry(struct dir_entry_info *entry, int free_target) } free(entry->name); + free(entry->xattr_value); free(entry); } @@ -551,6 +612,7 @@ static void remove_dir_entry(struct dir_entry_info *entry, int free_target) static int dir_new(struct dir_info *parent, const char *name) { char *path; + char *xattr_value; assert(parent); @@ -573,9 +635,11 @@ static int dir_new(struct dir_info *parent, const char *name) CHECK(lstat(path, &st) == 0); CHECK(S_ISDIR(st.st_mode)); } + xattr_value = assign_xattr(path); + CHECK(xattr_value != NULL); + add_dir_entry(parent, 'd', name, xattr_value, NULL); + free(xattr_value); free(path); - - add_dir_entry(parent, 'd', name, NULL); return 0; } @@ -630,6 +694,7 @@ static int dir_remove(struct dir_info *dir) static int file_new(struct dir_info *parent, const char *name) { char *path; + char *xattr_value; mode_t mode; int fd; @@ -657,7 +722,10 @@ static int file_new(struct dir_info *parent, const char *name) CHECK(S_ISREG(st.st_mode)); } - add_dir_entry(parent, 'f', name, NULL); + xattr_value = assign_xattr(path); + CHECK(xattr_value != NULL); + add_dir_entry(parent, 'f', name, xattr_value, NULL); + free(xattr_value); close(fd); free(path); @@ -669,6 +737,7 @@ static int link_new(struct dir_info *parent, const char *name, { struct dir_entry_info *entry; char *path, *target; + char *xattr_value = NULL; int ret; struct stat st1, st2; @@ -704,7 +773,10 @@ static int link_new(struct dir_info *parent, const char *name, CHECK(st2.st_nlink == st1.st_nlink + 1); } - add_dir_entry(parent, 'f', name, file); + xattr_value = assign_xattr(path); + CHECK(xattr_value != NULL); + add_dir_entry(parent, 'f', name, xattr_value, file); + free(xattr_value); free(target); free(path); @@ -1753,6 +1825,10 @@ static void dir_check(struct dir_info *dir) /* Now check each entry */ entry = dir->first; while (entry) { + char buf[2048]; + sprintf(buf, "%s/%s", path, entry->name); + if (entry->type == 'd' || entry->type == 'f') + xattr_check(buf, entry->xattr_value); if (entry->type == 'd') { dir_check(entry->dir); link_count += 1; /* <subdir>/.. */ @@ -1927,6 +2003,7 @@ static int rename_entry(struct dir_entry_info *entry) struct dir_entry_info *rename_entry = NULL; struct dir_info *parent; char *path, *to, *name; + char *xattr_value = NULL; int ret, isdir, retry; struct stat st1, st2; @@ -2000,7 +2077,8 @@ static int rename_entry(struct dir_entry_info *entry) return 0; } - add_dir_entry(parent, entry->type, name, entry->target); + add_dir_entry(parent, entry->type, name, xattr_value, entry->target); + free(xattr_value); if (rename_entry) remove_dir_entry(rename_entry, 1); remove_dir_entry(entry, 0); @@ -2132,7 +2210,7 @@ static int symlink_new(struct dir_info *dir, const char *nm) if (args.verify_ops) verify_symlink(target, path); - s = add_dir_entry(dir, 's', name, NULL); + s = add_dir_entry(dir, 's', name, NULL, NULL); s->target_pathname = target; free(path); diff --git a/tests/ubi-tests/Makefile b/tests/ubi-tests/Makefile index 2c47a9f..ba8e7e0 100644 --- a/tests/ubi-tests/Makefile +++ b/tests/ubi-tests/Makefile @@ -8,7 +8,8 @@ LIBS = libubi TARGETS=io_update volrefcnt integ io_paral io_read io_basic \ mkvol_basic mkvol_bad mkvol_paral rsvol -CFLAGS += -I$(LIBUBI_HEADER_PATH) -I $(KERNELHDR) -lpthread +CFLAGS += -I$(LIBUBI_HEADER_PATH) -I$(KERNELHDR) -lpthread +LDFLAGS += -lpthread include ../../common.mk
Changes since v1: Randomize value of extended attribute. Update dir_entry structure to keep track of extended attribute and check it during verification phase, as suggested during v1 review. Now compiling and running integck requires libattr. Signed-off-by: Subodh Nijsure <snijsure@grid-net.com> --- tests/fs-tests/integrity/Makefile | 2 +- tests/fs-tests/integrity/integck.c | 92 +++++++++++++++++++++++++++++++++--- tests/ubi-tests/Makefile | 3 +- 3 files changed, 88 insertions(+), 9 deletions(-)