Re: [PATCH] erofs-utils: support selinux file contexts

[Gao Xiang <hsiangkao@redhat.com>]

Hi Guifu,

On Wed, Jun 03, 2020 at 11:54:11PM +0800, Li GuiFu wrote:
> Gao Xiang
>   I have given some opinion
>   Please have a look, and resend a new one

It seems your email still didn't work properly since I don't
find that in the mailing list (as many other previous mail
providers of you...)

> 
> On 2020/5/31 0:11, Gao Xiang wrote:
> > Add --file-contexts flag that allows passing a selinux
> > file_context file to setup file selabels.
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---
> > prelimiary version.
> > 
> >  configure.ac           |  27 +++++++++
> >  include/erofs/config.h |  11 ++++
> >  include/erofs/xattr.h  |   3 +-
> >  lib/config.c           |  19 +++++++
> >  lib/exclude.c          |  12 +---
> >  lib/inode.c            |   3 +-
> >  lib/xattr.c            | 122 ++++++++++++++++++++++++++++++++---------
> >  man/mkfs.erofs.1       |   3 +
> >  mkfs/Makefile.am       |   4 +-
> >  mkfs/main.c            |  29 +++++++++-
> >  10 files changed, 190 insertions(+), 43 deletions(-)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 870dfb9..5145971 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -67,6 +67,15 @@ AC_ARG_WITH(uuid,
> >     [AS_HELP_STRING([--without-uuid],
> >        [Ignore presence of libuuid and disable uuid support @<:@default=enabled@:>@])])
> >  
> > +AC_ARG_WITH(selinux,
> > +   [AS_HELP_STRING([--with-selinux],
> > +      [enable and build with selinux support @<:@default=no@:>@])],
> > +   [case "$with_selinux" in
> > +      yes|no) ;;
> > +      *) AC_MSG_ERROR([invalid argument to --with-selinux])
> > +      ;;
> > +    esac], [with_selinux=no])
> > +
> >  # Checks for libraries.
> >  # Use customized LZ4 library path when specified.
> >  AC_ARG_WITH(lz4-incdir,
> > @@ -160,6 +169,20 @@ return 0;
> >    LIBS="${saved_LIBS}"
> >    CPPFLAGS="${saved_CPPFLAGS}"], [have_uuid="no"])
> >  
> > +# Configure selinux
> > +AS_IF([test "x$with_selinux" != "xno"], [
> > +  PKG_CHECK_MODULES([libselinux], [libselinux])
> > +  # Paranoia: don't trust the result reported by pkgconfig before trying out
> > +  saved_LIBS="$LIBS"
> > +  saved_CPPFLAGS=${CPPFLAGS}
> > +  CPPFLAGS="${libselinux_CFLAGS} ${CPPFLAGS}"
> > +  LIBS="${libselinux_LIBS} $LIBS"
> > +  AC_CHECK_LIB(selinux, selabel_lookup, [
> > +    have_selinux="yes" ], [
> > +    AC_MSG_ERROR([libselinux doesn't work properly])])
> > +  LIBS="${saved_LIBS}"
> > +  CPPFLAGS="${saved_CPPFLAGS}"], [have_selinux="no"])
> > +
> >  # Configure lz4
> >  test -z $LZ4_LIBS && LZ4_LIBS='-llz4'
> >  
> > @@ -199,6 +222,10 @@ if test "x$have_uuid" = "xyes"; then
> >    AC_DEFINE([HAVE_LIBUUID], 1, [Define to 1 if libuuid is found])
> >  fi
> >  
> > +if test "x$have_selinux" = "xyes"; then
> > +  AC_DEFINE([HAVE_LIBSELINUX], 1, [Define to 1 if libselinux is found])
> > +fi
> > +
> >  if test "x${have_lz4}" = "xyes"; then
> >    AC_DEFINE([LZ4_ENABLED], [1], [Define to 1 if lz4 is enabled.])
> >  
> Is it good to enable selinux support by default
> if the host has selinux installed  ?

Currently, configure.ac doesn't have --with-selinux=check option.

That means if "$with_selinux" == yes, it will fail if libselinux
doesn't find. That's for safety concern (just let users to install
if --with-selinux is enabled rather than build without libselinux).

Note that libselinux is an optional feature and unnecessary
for primary use cases. So users don't have to install it
apart from such as Android use cases.

> 
> > diff --git a/include/erofs/config.h b/include/erofs/config.h
> > index 2be05ee..825abaf 100644
> > --- a/include/erofs/config.h
> > +++ b/include/erofs/config.h
> > @@ -11,6 +11,11 @@
> >  
> >  #include "defs.h"
> >  
> > +#ifdef HAVE_LIBSELINUX
> > +#include <selinux/selinux.h>
> > +#include <selinux/label.h>
> > +#endif
> > +
> >  enum {
> >  	FORCE_INODE_COMPACT = 1,
> >  	FORCE_INODE_EXTENDED,
> > @@ -22,6 +27,9 @@ struct erofs_configure {
> >  	bool c_dry_run;
> >  	bool c_legacy_compress;
> >  
> > +#ifdef HAVE_LIBSELINUX
> > +	struct selabel_handle *sehnd;
> > +#endif
> >  	/* related arguments for mkfs.erofs */
> >  	char *c_img_path;
> >  	char *c_src_path;
> > @@ -39,5 +47,8 @@ void erofs_init_configure(void);
> >  void erofs_show_config(void);
> >  void erofs_exit_configure(void);
> >  
> > +void erofs_set_fs_root(const char *rootdir);
> > +const char *erofs_fspath(const char *fullpath);
> > +
> >  #endif
> >  
> > diff --git a/include/erofs/xattr.h b/include/erofs/xattr.h
> > index 3dff1ea..2e99669 100644
> > --- a/include/erofs/xattr.h
> > +++ b/include/erofs/xattr.h
> > @@ -42,7 +42,8 @@
> >  #define XATTR_NAME_POSIX_ACL_DEFAULT "system.posix_acl_default"
> >  #endif
> >  
> > -int erofs_prepare_xattr_ibody(const char *path, struct list_head *ixattrs);
> > +int erofs_prepare_xattr_ibody(const char *path, mode_t mode,
> > +			      struct list_head *ixattrs);
> >  char *erofs_export_xattr_ibody(struct list_head *ixattrs, unsigned int size);
> >  int erofs_build_shared_xattrs_from_path(const char *path);
> >  
> > diff --git a/lib/config.c b/lib/config.c
> > index cbbecce..ae21252 100644
> > --- a/lib/config.c
> > +++ b/lib/config.c
> > @@ -36,6 +36,25 @@ void erofs_show_config(void)
> >  
> >  void erofs_exit_configure(void)
> >  {
> > +#ifdef HAVE_LIBSELINUX
> > +	if (cfg.sehnd)
> > +		selabel_close(cfg.sehnd);
> > +#endif
> > +}
> > +
> > +static unsigned int fullpath_prefix;	/* root directory prefix length */
> > +
> > +void erofs_set_fs_root(const char *rootdir)
> > +{
> > +	fullpath_prefix = strlen(rootdir);
> > +}
> > +
> > +const char *erofs_fspath(const char *fullpath)
> > +{
> > +	const char *s = fullpath + fullpath_prefix;
> >  
> > +	while (*s == '/')
> > +		s++;
> > +	return s;
> >  }
> >  
> > diff --git a/lib/exclude.c b/lib/exclude.c
> > index 47b467d..73b3720 100644
> > --- a/lib/exclude.c
> > +++ b/lib/exclude.c
> > @@ -17,13 +17,6 @@
> >  static LIST_HEAD(exclude_head);
> >  static LIST_HEAD(regex_exclude_head);
> >  
> > -static unsigned int rpathlen;		/* root directory prefix length */
> > -
> > -void erofs_exclude_set_root(const char *rootdir)
> > -{
> > -	rpathlen = strlen(rootdir);
> > -}
> > -
> >  static void dump_regerror(int errcode, const char *s, const regex_t *preg)
> >  {
> >  	char str[512];
> > @@ -120,10 +113,7 @@ struct erofs_exclude_rule *erofs_is_exclude_path(const char *dir,
> >  		s = buf;
> >  	}
> >  
> > -	s += rpathlen;
> > -	while (*s == '/')
> > -		s++;
> > -
> > +	s = erofs_fspath(s);
> >  	list_for_each_entry(r, &exclude_head, list) {
> >  		if (!strcmp(r->pattern, s))
> >  			return r;
> > diff --git a/lib/inode.c b/lib/inode.c
> > index 7114023..dff5f2c 100644
> > --- a/lib/inode.c
> > +++ b/lib/inode.c
> > @@ -827,7 +827,8 @@ struct erofs_inode *erofs_mkfs_build_tree(struct erofs_inode *dir)
> >  	struct dirent *dp;
> >  	struct erofs_dentry *d;
> >  
> > -	ret = erofs_prepare_xattr_ibody(dir->i_srcpath, &dir->i_xattrs);
> > +	ret = erofs_prepare_xattr_ibody(dir->i_srcpath,
> > +					dir->i_mode, &dir->i_xattrs);
> >  	if (ret < 0)
> >  		return ERR_PTR(ret);
> >  	dir->xattr_isize = ret;
> > diff --git a/lib/xattr.c b/lib/xattr.c
> > index 1564016..698c237 100644
> > --- a/lib/xattr.c
> > +++ b/lib/xattr.c
> > @@ -186,6 +186,43 @@ static struct xattr_item *parse_one_xattr(const char *path, const char *key,
> >  	return get_xattritem(prefix, kvbuf, len);
> >  }
> >  
> > +static struct xattr_item *erofs_get_selabel_xattr(const char *srcpath,
> > +						  mode_t mode)
> > +{
> > +#ifdef HAVE_LIBSELINUX
> > +	if (cfg.sehnd) {
> > +		char *secontext;
> > +		int ret;
> > +		unsigned int len[2];
> > +		char *kvbuf, *fspath;
> > +
> > +		ret = asprintf(&fspath, "/%s", erofs_fspath(srcpath));
> > +		if (ret <= 0)
> > +			return ERR_PTR(-ENOMEM);
> > +
> > +		ret = selabel_lookup(cfg.sehnd, &secontext, fspath, mode);
> > +		free(fspath);
> > +
> > +		if (ret) {
> > +			ret = -errno;
> > +			erofs_err("cannot lookup selabel for %s", srcpath);
> > +			/* secontext = strdup("u:object_r:unlabeled:s0"); */
> > +			return ERR_PTR(ret);
> > +		}
> > +
> > +		len[0] = sizeof("selinux") - 1;
> > +		len[1] = strlen(secontext);
> > +		kvbuf = malloc(len[0] + len[1] + 1);
> > +		if (!kvbuf)
> > +			return ERR_PTR(-ENOMEM);
> > +
> > +		sprintf(kvbuf, "selinux%s", secontext);
> > +		return get_xattritem(EROFS_XATTR_INDEX_SECURITY, kvbuf, len);
> > +	}
> > +#endif
> > +	return NULL;
> > +}
> > +
> >  static int inode_xattr_add(struct list_head *hlist, struct xattr_item *item)
> >  {
> >  	struct inode_xattr_node *node = malloc(sizeof(*node));
> > @@ -215,19 +252,48 @@ static int shared_xattr_add(struct xattr_item *item)
> >  	return ++shared_xattrs_count;
> >  }
> >  
> > -static int read_xattrs_from_file(const char *path, struct list_head *ixattrs)
> > +static int erofs_xattr_add(struct list_head *ixattrs, struct xattr_item *item)
> > +{
> > +	if (ixattrs)
> > +		return inode_xattr_add(ixattrs, item);
> > +
> > +	if (item->count == cfg.c_inline_xattr_tolerance + 1) {
> > +		int ret = shared_xattr_add(item);
> > +
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static bool erofs_is_skipped_xattr(const char *key)
> > +{
> > +#ifdef HAVE_LIBSELINUX
> > +	/* if sehnd is valid, selabels will be overridden */
> > +	if (cfg.sehnd && !strcmp(key, XATTR_SECURITY_PREFIX "selinux"))
> > +		return true;
> > +#endif
> > +	return false;
> > +}
> > +
> > +static int read_xattrs_from_file(const char *path, mode_t mode,
> > +				 struct list_head *ixattrs)
> >  {
> > -	int ret = 0;
> > -	char *keylst, *key;
> >  	ssize_t kllen = llistxattr(path, NULL, 0);
> > +	int ret;
> > +	char *keylst, *key, *klend;
> > +	unsigned int keylen;
> > +	struct xattr_item *item;
> >  
> >  	if (kllen < 0 && errno != ENODATA) {
> >  		erofs_err("llistxattr to get the size of names for %s failed",
> >  			  path);
> >  		return -errno;
> >  	}
> > +
> > +	ret = 0;
> >  	if (kllen <= 1)
> > -		return 0;
> > +		goto out;
> >  
> >  	keylst = malloc(kllen);
> >  	if (!keylst)
> > @@ -246,36 +312,38 @@ static int read_xattrs_from_file(const char *path, struct list_head *ixattrs)
> >  	 * attribute keys. Use the remaining buffer length to determine
> >  	 * the end of the list.
> >  	 */
> > -	key = keylst;
> > -	while (kllen > 0) {
> > -		unsigned int keylen = strlen(key);
> > -		struct xattr_item *item = parse_one_xattr(path, key, keylen);
> > +	klend = keylst + kllen;
> > +	ret = 0;
> > +
> > +	for (key = keylst; key != klend; key += keylen + 1) {
> > +		keylen = strlen(key);
> > +		if (erofs_is_skipped_xattr(key))
> > +			continue;
> >  
> > +		item = parse_one_xattr(path, key, keylen);
> >  		if (IS_ERR(item)) {
> >  			ret = PTR_ERR(item);
> >  			goto err;
> >  		}
> >  
> > -		if (ixattrs) {
> > -			ret = inode_xattr_add(ixattrs, item);
> > -			if (ret < 0)
> > -				goto err;
> > -		} else if (item->count == cfg.c_inline_xattr_tolerance + 1) {
> > -			ret = shared_xattr_add(item);
> > -			if (ret < 0)
> > -				goto err;
> > -			ret = 0;
> > -		}
> > -		kllen -= keylen + 1;
> > -		key += keylen + 1;
> > +		ret = erofs_xattr_add(ixattrs, item);
> > +		if (ret < 0)
> > +			goto err;
> >  	}
> > -err:
> >  	free(keylst);
> > +out:
> > +	item = erofs_get_selabel_xattr(path, mode);
> > +	if (item)
> > +		ret = erofs_xattr_add(ixattrs, item);
> item may cause a segment fault if erofs_get_selabel_xattr goes fail
> which return a ERR_PTR.

Good catch! will fix it soon. Thank you!

> 
> Does it need create a new function detach from xattr ?
> So read_xattrs_from_file just do one thing

These short part only have one user for now and it's really a part of
generating xattrs for a file. So I think it's unnecessary to create
a new helper for this. (I'd like to avoid too many one-shot short
helpers since I don't think that is a good habit. You know that
readers will look into an extra function and go back to trace the
whole path, that seems redundant. As for better understanding,
I think we can leave some comments here.)

> 
> >  	return ret;
> >  
> > +err:
> > +	free(keylst);
> > +	return ret;
> >  }
> >  
> > -int erofs_prepare_xattr_ibody(const char *path, struct list_head *ixattrs)
> > +int erofs_prepare_xattr_ibody(const char *path, mode_t mode,
> > +			      struct list_head *ixattrs)
> >  {
> >  	int ret;
> >  	struct inode_xattr_node *node;
> > @@ -284,7 +352,7 @@ int erofs_prepare_xattr_ibody(const char *path, struct list_head *ixattrs)
> >  	if (cfg.c_inline_xattr_tolerance < 0)
> >  		return 0;
> >  
> > -	ret = read_xattrs_from_file(path, ixattrs);
> > +	ret = read_xattrs_from_file(path, mode, ixattrs);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -345,16 +413,16 @@ static int erofs_count_all_xattrs_from_path(const char *path)
> >  			goto fail;
> >  		}
> >  
> > -		ret = read_xattrs_from_file(buf, NULL);
> > -		if (ret)
> > -			goto fail;
> > -
> >  		ret = lstat64(buf, &st);
> >  		if (ret) {
> >  			ret = -errno;
> >  			goto fail;
> >  		}
> >  
> > +		ret = read_xattrs_from_file(buf, st.st_mode, NULL);
> > +		if (ret)
> > +			goto fail;
> > +
> >  		if (!S_ISDIR(st.st_mode))
> >  			continue;
> >  
> > diff --git a/man/mkfs.erofs.1 b/man/mkfs.erofs.1
> > index d47207a..891c5a8 100644
> > --- a/man/mkfs.erofs.1
> > +++ b/man/mkfs.erofs.1
> > @@ -60,6 +60,9 @@ You may give multiple `--exclude-path' options.
> >  Ignore files that match the given regular expression.
> >  You may give multiple `--exclude-regex` options.
> >  .TP
> > +.BI "\-\-file-contexts=" file
> > +Specify a \fIfile_contexts\fR file to setup / override selinux labels.
> > +.TP
> >  .B \-\-help
> >  Display this help and exit.
> >  .SH AUTHOR
> > diff --git a/mkfs/Makefile.am b/mkfs/Makefile.am
> > index 9ce06d6..97ba148 100644
> > --- a/mkfs/Makefile.am
> > +++ b/mkfs/Makefile.am
> > @@ -3,8 +3,8 @@
> >  
> >  AUTOMAKE_OPTIONS = foreign
> >  bin_PROGRAMS     = mkfs.erofs
> > -AM_CPPFLAGS = ${libuuid_CFLAGS}
> > +AM_CPPFLAGS = ${libuuid_CFLAGS} ${libselinux_CFLAGS}
> >  mkfs_erofs_SOURCES = main.c
> >  mkfs_erofs_CFLAGS = -Wall -Werror -I$(top_srcdir)/include
> > -mkfs_erofs_LDADD = $(top_builddir)/lib/liberofs.la ${libuuid_LIBS}
> > +mkfs_erofs_LDADD = $(top_builddir)/lib/liberofs.la ${libuuid_LIBS} ${libselinux_LIBS}
> >  
> > diff --git a/mkfs/main.c b/mkfs/main.c
> > index 940d4e8..7de929f 100644
> > --- a/mkfs/main.c
> > +++ b/mkfs/main.c
> > @@ -33,6 +33,9 @@ static struct option long_options[] = {
> >  	{"help", no_argument, 0, 1},
> >  	{"exclude-path", required_argument, NULL, 2},
> >  	{"exclude-regex", required_argument, NULL, 3},
> > +#ifdef HAVE_LIBSELINUX
> > +	{"file-contexts", required_argument, NULL, 4},
> > +#endif
> 
> Many other fs tool use "-S" to set file contexts file path,
> What's you opinon to add a short option ?

"--file-contexts" is a straight-forward way for understanding
this concept. "-S" should have more common and useful meanings,
such as "sparse" or "strip", or "size" (but usually in low-case
"-s"). I'd like to keep some more common meaning in sync with
other tools.

I know your mean make_ext4fs has "-S" option for selinux file
contexts file. but make_ext4fs was an Android only tool (which
was also deprecated by using e2fsprogs). Also, as you can see
what Google did for squashfs-tools, they used a long option
at that time.

If you consider some HUAWEI Android in-house downstream
compatibility, I'd suggest to modify build scripts for this.
Anyway, I'd like to upstream erofs for AOSP. But we need to
complete all remaining Android-specific stuffs (such as fs
config, Android.bp, build scripts) in advance.

> 
> >  	{0, 0, 0, 0},
> >  };
> >  
> > @@ -60,6 +63,9 @@ static void usage(void)
> >  	      " -T#               set a fixed UNIX timestamp # to all files\n"
> >  	      " --exclude-path=X  avoid including file X (X = exact literal path)\n"
> >  	      " --exclude-regex=X avoid including files that match X (X = regular expression)\n"
> > +#ifdef HAVE_LIBSELINUX
> > +	      " --file-contexts=X specify a file contexts file to setup selinux labels\n"
> > +#endif
> >  	      " --help            display this help and exit\n"
> >  	      "\nAvailable compressors are: ", stderr);
> >  	print_available_compressors(stderr, ", ");
> > @@ -198,6 +204,27 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
> >  				return opt;
> >  			}
> >  			break;
> > +
> > +#ifdef HAVE_LIBSELINUX
> > +		case 4: {
> > +			struct selinux_opt seopts[] = {
> > +				{ .type = SELABEL_OPT_PATH, .value = optarg }
> > +			};
> > +
> > +			if (cfg.sehnd) {
> > +				erofs_info("ignore duplicated file contexts \"%s\"",
> > +					   optarg);
> > +				break;
> > +			}
> > +			cfg.sehnd = selabel_open(SELABEL_CTX_FILE, seopts, 1);
> > +			if (!cfg.sehnd) {
> > +				erofs_err("failed to open file contexts \"%s\"",
> > +					  optarg);
> > +				return -EINVAL;
> > +			}
> > +			break;
> > +		}
> > +#endif
> it seems  more clear and not complex if move selable_open to xattr.c file.

Okay, will move it to config.c.

Thanks,
Gao Xiang

> 
> 
> >  		case 1:
> >  			usage();
> >  			exit(0);
> > @@ -392,7 +419,7 @@ int main(int argc, char **argv)
> >  	}
> >  
> >  	erofs_show_config();
> > -	erofs_exclude_set_root(cfg.c_src_path);
> > +	erofs_set_fs_root(cfg.c_src_path);
> >  
> >  	sb_bh = erofs_buffer_init();
> >  	if (IS_ERR(sb_bh)) {
> > 
>