[fsverity-utils PATCH 1/2] Remove unneeded includes

[fsverity-utils PATCH 1/2] Remove unneeded includes

[luca.boccassi]

From: Luca Boccassi <luca.boccassi@microsoft.com>

Signed-off-by: Luca Boccassi <luca.boccassi@microsoft.com>
---
 common/fsverity_uapi.h | 1 -
 programs/cmd_enable.c  | 1 -
 2 files changed, 2 deletions(-)

diff --git a/common/fsverity_uapi.h b/common/fsverity_uapi.h
index 33f4415..0006c35 100644
--- a/common/fsverity_uapi.h
+++ b/common/fsverity_uapi.h
@@ -10,7 +10,6 @@
 #ifndef _UAPI_LINUX_FSVERITY_H
 #define _UAPI_LINUX_FSVERITY_H
 
-#include <linux/ioctl.h>
 #include <linux/types.h>
 
 #define FS_VERITY_HASH_ALG_SHA256	1
diff --git a/programs/cmd_enable.c b/programs/cmd_enable.c
index fdf26c7..14c3c17 100644
--- a/programs/cmd_enable.c
+++ b/programs/cmd_enable.c
@@ -14,7 +14,6 @@
 #include <fcntl.h>
 #include <getopt.h>
 #include <limits.h>
-#include <sys/ioctl.h>
 
 static bool read_signature(const char *filename, u8 **sig_ret,
 			   u32 *sig_size_ret)
-- 
2.29.2

[fsverity-utils PATCH 2/2] Allow to build and run sign/digest on Windows

[luca.boccassi]

From: Luca Boccassi <luca.boccassi@microsoft.com>

Add some minimal compat type defs, and stub out the enable/measure
functions. Also add a way to handle the fact that mingw adds a
.exe extension automatically in the Makefile install rules.

Signed-off-by: Luca Boccassi <luca.boccassi@microsoft.com>
---
So this is proably going to look strange, and believe me the feeling is shared.
It's actually the first time _ever_ I had to compile and run something on
Windows, which is ironic in itself - the O_BINARY stuff is probably WIN32-101 and
it took me an hour to find out.
Anyway, I've got some groups building their payloads on Windows, so we need to
provide native tooling. Among these are fsverity tools to get the digest and
sign files.
This patch stubs out and returns EOPNOTSUPP from the measure/enable functions,
since they are linux-host only, and adds some (hopefully) minimal and unintrusive
compat ifdefs for the rest. There's also a change in the makefile, since the
build toolchain (yocto + mingw) for some reason automatically names executables
as .exe. Biggest chunk is the types definitions I guess. The ugliest is the
print stuff.

Note that with this I do not ask you in any way, shape or form to be responsible
for the correct functioning or even compiling on WIN32 of these utilities - if
anything ever breaks, we'll find out and take care of it. I could keep all of this
out of tree, but I figured I'd try to see if you are amenable to merge at least
some part of it.

I've tested that both Linux and WIN32 builds of digest and sign commands return
the exact same output for the same input.

 Makefile               | 13 +++++++------
 common/common_defs.h   | 24 ++++++++++++++++++++++++
 common/fsverity_uapi.h |  4 ++++
 lib/enable.c           | 27 +++++++++++++++++++++------
 lib/utils.c            | 17 ++++++++++++++++-
 programs/cmd_measure.c | 13 +++++++++++++
 programs/utils.c       |  7 ++++++-
 7 files changed, 91 insertions(+), 14 deletions(-)

diff --git a/Makefile b/Makefile
index bfe83c4..fe89e18 100644
--- a/Makefile
+++ b/Makefile
@@ -63,6 +63,7 @@ INCDIR          ?= $(PREFIX)/include
 LIBDIR          ?= $(PREFIX)/lib
 DESTDIR         ?=
 PKGCONF         ?= pkg-config
+EXEEXT          ?=
 
 # Rebuild if a user-specified setting that affects the build changed.
 .build-config: FORCE
@@ -87,9 +88,9 @@ CFLAGS          += $(shell "$(PKGCONF)" libcrypto --cflags 2>/dev/null || echo)
 # If we are dynamically linking, when running tests we need to override
 # LD_LIBRARY_PATH as no RPATH is set
 ifdef USE_SHARED_LIB
-RUN_FSVERITY    = LD_LIBRARY_PATH=./ ./fsverity
+RUN_FSVERITY    = LD_LIBRARY_PATH=./ ./fsverity$(EXEEXT)
 else
-RUN_FSVERITY    = ./fsverity
+RUN_FSVERITY    = ./fsverity$(EXEEXT)
 endif
 
 ##############################################################################
@@ -186,7 +187,7 @@ test_programs:$(TEST_PROGRAMS)
 # want to run the full tests.
 check:fsverity test_programs
 	for prog in $(TEST_PROGRAMS); do \
-		$(TEST_WRAPPER_PROG) ./$$prog || exit 1; \
+		$(TEST_WRAPPER_PROG) ./$$prog$(EXEEXT) || exit 1; \
 	done
 	$(RUN_FSVERITY) --help > /dev/null
 	$(RUN_FSVERITY) --version > /dev/null
@@ -202,7 +203,7 @@ check:fsverity test_programs
 
 install:all
 	install -d $(DESTDIR)$(LIBDIR)/pkgconfig $(DESTDIR)$(INCDIR) $(DESTDIR)$(BINDIR)
-	install -m755 fsverity $(DESTDIR)$(BINDIR)
+	install -m755 fsverity$(EXEEXT) $(DESTDIR)$(BINDIR)
 	install -m644 libfsverity.a $(DESTDIR)$(LIBDIR)
 	install -m755 libfsverity.so.$(SOVERSION) $(DESTDIR)$(LIBDIR)
 	ln -sf libfsverity.so.$(SOVERSION) $(DESTDIR)$(LIBDIR)/libfsverity.so
@@ -215,7 +216,7 @@ install:all
 	chmod 644 $(DESTDIR)$(LIBDIR)/pkgconfig/libfsverity.pc
 
 uninstall:
-	rm -f $(DESTDIR)$(BINDIR)/fsverity
+	rm -f $(DESTDIR)$(BINDIR)/fsverity$(EXEEXT)
 	rm -f $(DESTDIR)$(LIBDIR)/libfsverity.a
 	rm -f $(DESTDIR)$(LIBDIR)/libfsverity.so.$(SOVERSION)
 	rm -f $(DESTDIR)$(LIBDIR)/libfsverity.so
@@ -232,4 +233,4 @@ help:
 
 clean:
 	rm -f $(DEFAULT_TARGETS) $(TEST_PROGRAMS) \
-		lib/*.o programs/*.o .build-config fsverity.sig
+		fsverity$(EXEEXT) lib/*.o programs/*.o .build-config fsverity.sig
diff --git a/common/common_defs.h b/common/common_defs.h
index 279385a..a869532 100644
--- a/common/common_defs.h
+++ b/common/common_defs.h
@@ -15,6 +15,30 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#ifdef _WIN32
+/* Some minimal definitions to allow the digest/sign commands to run under Windows */
+#  ifndef ENOPKG
+#    define ENOPKG 65
+#  endif
+#  ifndef __cold
+#    define __cold
+#  endif
+typedef __signed__ char __s8;
+typedef unsigned char __u8;
+typedef __signed__ short __s16;
+typedef unsigned short __u16;
+typedef __signed__ int __s32;
+typedef unsigned int __u32;
+typedef __signed__ long long  __s64;
+typedef unsigned long long  __u64;
+typedef __u16 __le16;
+typedef __u16 __be16;
+typedef __u32 __le32;
+typedef __u32 __be32;
+typedef __u64 __le64;
+typedef __u64 __be64;
+#endif /* _WIN32 */
+
 typedef uint8_t u8;
 typedef uint16_t u16;
 typedef uint32_t u32;
diff --git a/common/fsverity_uapi.h b/common/fsverity_uapi.h
index 0006c35..ec8294a 100644
--- a/common/fsverity_uapi.h
+++ b/common/fsverity_uapi.h
@@ -10,7 +10,11 @@
 #ifndef _UAPI_LINUX_FSVERITY_H
 #define _UAPI_LINUX_FSVERITY_H
 
+#ifndef _WIN32
 #include <linux/types.h>
+#else
+#include "common_defs.h"
+#endif /* _WIN32 */
 
 #define FS_VERITY_HASH_ALG_SHA256	1
 #define FS_VERITY_HASH_ALG_SHA512	2
diff --git a/lib/enable.c b/lib/enable.c
index 2478077..b49ba26 100644
--- a/lib/enable.c
+++ b/lib/enable.c
@@ -11,14 +11,10 @@
 
 #include "lib_private.h"
 
+#ifndef _WIN32
+
 #include <sys/ioctl.h>
 
-LIBEXPORT int
-libfsverity_enable(int fd, const struct libfsverity_merkle_tree_params *params)
-{
-	return libfsverity_enable_with_sig(fd, params, NULL, 0);
-}
-
 LIBEXPORT int
 libfsverity_enable_with_sig(int fd,
 			    const struct libfsverity_merkle_tree_params *params,
@@ -51,3 +47,22 @@ libfsverity_enable_with_sig(int fd,
 		return -errno;
 	return 0;
 }
+
+#else /* _WIN32 */
+
+LIBEXPORT int
+libfsverity_enable_with_sig(__attribute__ ((unused)) int fd,
+			    __attribute__ ((unused)) const struct libfsverity_merkle_tree_params *params,
+			    __attribute__ ((unused)) const uint8_t *sig,
+			    __attribute__ ((unused)) size_t sig_size)
+{
+	return -EOPNOTSUPP;
+}
+
+#endif /* _WIN32 */
+
+LIBEXPORT int
+libfsverity_enable(int fd, const struct libfsverity_merkle_tree_params *params)
+{
+	return libfsverity_enable_with_sig(fd, params, NULL, 0);
+}
diff --git a/lib/utils.c b/lib/utils.c
index 8b5d6cb..2ff8fd2 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -10,6 +10,16 @@
  */
 
 #define _GNU_SOURCE /* for asprintf() and strerror_r() */
+#ifndef _WIN32
+#  define SIZET_PF "zu"
+#else
+#  include <inttypes.h>
+#  ifdef _WIN64
+#    define SIZET_PF PRIu64
+#  else
+#    define SIZET_PF PRIu32
+#  endif
+#endif
 
 #include "lib_private.h"
 
@@ -22,7 +32,7 @@ static void *xmalloc(size_t size)
 	void *p = malloc(size);
 
 	if (!p)
-		libfsverity_error_msg("out of memory (tried to allocate %zu bytes)",
+		libfsverity_error_msg("out of memory (tried to allocate %" SIZET_PF " bytes)",
 				      size);
 	return p;
 }
@@ -68,8 +78,13 @@ void libfsverity_do_error_msg(const char *format, va_list va, int err)
 		char *msg2 = NULL;
 		char errbuf[64];
 
+#ifndef _WIN32
 		if (asprintf(&msg2, "%s: %s", msg,
 			     strerror_r(err, errbuf, sizeof(errbuf))) < 0)
+#else
+		strerror_s(errbuf, sizeof(errbuf), err);
+		if (asprintf(&msg2, "%s: %s", msg, errbuf) < 0)
+#endif
 			goto out2;
 		free(msg);
 		msg = msg2;
diff --git a/programs/cmd_measure.c b/programs/cmd_measure.c
index d78969c..48161d5 100644
--- a/programs/cmd_measure.c
+++ b/programs/cmd_measure.c
@@ -11,6 +11,8 @@
 
 #include "fsverity.h"
 
+#ifndef _WIN32
+
 #include <fcntl.h>
 #include <sys/ioctl.h>
 
@@ -67,3 +69,14 @@ out_usage:
 	status = 2;
 	goto out;
 }
+
+#else /* _WIN32 */
+
+int fsverity_cmd_measure(__attribute__ ((unused)) const struct fsverity_command *cmd,
+			 __attribute__ ((unused)) int argc,
+			 __attribute__ ((unused)) char *argv[])
+{
+	return -EOPNOTSUPP;
+}
+
+#endif /* _WIN32 */
diff --git a/programs/utils.c b/programs/utils.c
index facccda..fbcf248 100644
--- a/programs/utils.c
+++ b/programs/utils.c
@@ -18,6 +18,11 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+/* All file reads we do need this flag on _WIN32 */
+#ifndef O_BINARY
+#define O_BINARY 0
+#endif
+
 /* ========== Memory allocation ========== */
 
 void *xmalloc(size_t size)
@@ -102,7 +107,7 @@ void install_libfsverity_error_handler(void)
 
 bool open_file(struct filedes *file, const char *filename, int flags, int mode)
 {
-	file->fd = open(filename, flags, mode);
+	file->fd = open(filename, flags | O_BINARY, mode);
 	if (file->fd < 0) {
 		error_msg_errno("can't open '%s' for %s", filename,
 				(flags & O_ACCMODE) == O_RDONLY ? "reading" :
-- 
2.29.2

Re: [fsverity-utils PATCH 1/2] Remove unneeded includes

[Eric Biggers]

On Wed, Dec 16, 2020 at 05:27:18PM +0000, luca.boccassi@gmail.com wrote:
> From: Luca Boccassi <luca.boccassi@microsoft.com>
> 
> Signed-off-by: Luca Boccassi <luca.boccassi@microsoft.com>
> ---
>  common/fsverity_uapi.h | 1 -
>  programs/cmd_enable.c  | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/common/fsverity_uapi.h b/common/fsverity_uapi.h
> index 33f4415..0006c35 100644
> --- a/common/fsverity_uapi.h
> +++ b/common/fsverity_uapi.h
> @@ -10,7 +10,6 @@
>  #ifndef _UAPI_LINUX_FSVERITY_H
>  #define _UAPI_LINUX_FSVERITY_H
>  
> -#include <linux/ioctl.h>
>  #include <linux/types.h>

fsverity_uapi.h is supposed to be kept in sync with
include/uapi/linux/fsverity.h in the kernel source tree.

There's a reason why it includes <linux/ioctl.h>.  <linux/ioctl.h> provides the
_IOW() and _IOWR() macros to expand the values of FS_IOC_ENABLE_VERITY and
FS_IOC_MEASURE_VERITY.  Usually people referring to FS_IOC_* will include
<sys/ioctl.h> in order to actually call ioctl() too.  However it's not
guaranteed, so technically the header needs to include <linux/ioctl.h>.

Wrapping this include with '#ifdef _WIN32' would be better than removing it, as
then it would be clear that it's a Windows-specific modification.

However I think an even better approach would be to add empty files
win32-headers/linux/{types,ioctl.h} and add -Iwin32-headers to CPPFLAGS, so that
these headers can still be included in the Windows build without having to
modify the source files.

> diff --git a/programs/cmd_enable.c b/programs/cmd_enable.c
> index fdf26c7..14c3c17 100644
> --- a/programs/cmd_enable.c
> +++ b/programs/cmd_enable.c
> @@ -14,7 +14,6 @@
>  #include <fcntl.h>
>  #include <getopt.h>
>  #include <limits.h>
> -#include <sys/ioctl.h>
>  

This part looks fine though, as cmd_enable.c no longer directly uses an ioctl.

- Eric

Re: [fsverity-utils PATCH 2/2] Allow to build and run sign/digest on Windows

[Eric Biggers]

On Wed, Dec 16, 2020 at 05:27:19PM +0000, luca.boccassi@gmail.com wrote:
> From: Luca Boccassi <luca.boccassi@microsoft.com>
> 
> Add some minimal compat type defs, and stub out the enable/measure
> functions. Also add a way to handle the fact that mingw adds a
> .exe extension automatically in the Makefile install rules.
> 
> Signed-off-by: Luca Boccassi <luca.boccassi@microsoft.com>
> ---
> So this is proably going to look strange, and believe me the feeling is shared.
> It's actually the first time _ever_ I had to compile and run something on
> Windows, which is ironic in itself - the O_BINARY stuff is probably WIN32-101 and
> it took me an hour to find out.
> Anyway, I've got some groups building their payloads on Windows, so we need to
> provide native tooling. Among these are fsverity tools to get the digest and
> sign files.
> This patch stubs out and returns EOPNOTSUPP from the measure/enable functions,
> since they are linux-host only, and adds some (hopefully) minimal and unintrusive
> compat ifdefs for the rest. There's also a change in the makefile, since the
> build toolchain (yocto + mingw) for some reason automatically names executables
> as .exe. Biggest chunk is the types definitions I guess. The ugliest is the
> print stuff.
> 
> Note that with this I do not ask you in any way, shape or form to be responsible
> for the correct functioning or even compiling on WIN32 of these utilities - if
> anything ever breaks, we'll find out and take care of it. I could keep all of this
> out of tree, but I figured I'd try to see if you are amenable to merge at least
> some part of it.
> 
> I've tested that both Linux and WIN32 builds of digest and sign commands return
> the exact same output for the same input.

On Linux, can you make it easily cross-compile for Windows using
'make CC=x86_64-w64-mingw32-gcc'?  That would be ideal, so that that command can
be added to scripts/run-tests.sh, so that I can make sure it stays building.

I probably won't actually test *running* it on Windows, as that would be more
work.  But building should be easy.

> diff --git a/Makefile b/Makefile
> index bfe83c4..fe89e18 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -63,6 +63,7 @@ INCDIR          ?= $(PREFIX)/include
>  LIBDIR          ?= $(PREFIX)/lib
>  DESTDIR         ?=
>  PKGCONF         ?= pkg-config
> +EXEEXT          ?=

It looks like you're requiring the caller to manually specify EXEEXT.  You could
use something like the following to automatically detect when CC is MinGW and
set EXEEXT and AR appropriately:

# Compiling for Windows with MinGW?
ifneq ($(findstring -mingw,$(shell $(CC) -dumpmachine 2>/dev/null)),)
	EXEEXT := .exe
	# Derive $(AR) from $(CC).
	AR := $(shell echo $(CC) | \
                sed -E 's/g?cc(-?[0-9]+(\.[0-9]+)*)?(\.exe)?$$/ar\3/')
endif

>  # Rebuild if a user-specified setting that affects the build changed.
>  .build-config: FORCE
> @@ -87,9 +88,9 @@ CFLAGS          += $(shell "$(PKGCONF)" libcrypto --cflags 2>/dev/null || echo)
>  # If we are dynamically linking, when running tests we need to override
>  # LD_LIBRARY_PATH as no RPATH is set
>  ifdef USE_SHARED_LIB
> -RUN_FSVERITY    = LD_LIBRARY_PATH=./ ./fsverity
> +RUN_FSVERITY    = LD_LIBRARY_PATH=./ ./fsverity$(EXEEXT)
>  else
> -RUN_FSVERITY    = ./fsverity
> +RUN_FSVERITY    = ./fsverity$(EXEEXT)
>  endif
>  
>  ##############################################################################
> @@ -186,7 +187,7 @@ test_programs:$(TEST_PROGRAMS)
>  # want to run the full tests.
>  check:fsverity test_programs
>  	for prog in $(TEST_PROGRAMS); do \
> -		$(TEST_WRAPPER_PROG) ./$$prog || exit 1; \
> +		$(TEST_WRAPPER_PROG) ./$$prog$(EXEEXT) || exit 1; \
>  	done
>  	$(RUN_FSVERITY) --help > /dev/null
>  	$(RUN_FSVERITY) --version > /dev/null
> @@ -202,7 +203,7 @@ check:fsverity test_programs
>  
>  install:all
>  	install -d $(DESTDIR)$(LIBDIR)/pkgconfig $(DESTDIR)$(INCDIR) $(DESTDIR)$(BINDIR)
> -	install -m755 fsverity $(DESTDIR)$(BINDIR)
> +	install -m755 fsverity$(EXEEXT) $(DESTDIR)$(BINDIR)
>  	install -m644 libfsverity.a $(DESTDIR)$(LIBDIR)
>  	install -m755 libfsverity.so.$(SOVERSION) $(DESTDIR)$(LIBDIR)
>  	ln -sf libfsverity.so.$(SOVERSION) $(DESTDIR)$(LIBDIR)/libfsverity.so
> @@ -215,7 +216,7 @@ install:all
>  	chmod 644 $(DESTDIR)$(LIBDIR)/pkgconfig/libfsverity.pc
>  
>  uninstall:
> -	rm -f $(DESTDIR)$(BINDIR)/fsverity
> +	rm -f $(DESTDIR)$(BINDIR)/fsverity$(EXEEXT)
>  	rm -f $(DESTDIR)$(LIBDIR)/libfsverity.a
>  	rm -f $(DESTDIR)$(LIBDIR)/libfsverity.so.$(SOVERSION)
>  	rm -f $(DESTDIR)$(LIBDIR)/libfsverity.so
> @@ -232,4 +233,4 @@ help:
>  
>  clean:
>  	rm -f $(DEFAULT_TARGETS) $(TEST_PROGRAMS) \
> -		lib/*.o programs/*.o .build-config fsverity.sig
> +		fsverity$(EXEEXT) lib/*.o programs/*.o .build-config fsverity.sig

Do you need libfsverity to be built properly for Windows (producing .dll, .lib,
and .def files), or are you just looking to build the fsverity binary?  At the
moment you're just doing the latter.  There are a bunch of differences between
libraries on Windows and Linux, so hopefully you don't need the library built
properly for Windows, but it would be possible.

> diff --git a/common/common_defs.h b/common/common_defs.h
> index 279385a..a869532 100644
> --- a/common/common_defs.h
> +++ b/common/common_defs.h
> @@ -15,6 +15,30 @@
>  #include <stddef.h>
>  #include <stdint.h>
>  
> +#ifdef _WIN32
> +/* Some minimal definitions to allow the digest/sign commands to run under Windows */
> +#  ifndef ENOPKG
> +#    define ENOPKG 65
> +#  endif
> +#  ifndef __cold
> +#    define __cold
> +#  endif
> +typedef __signed__ char __s8;
> +typedef unsigned char __u8;
> +typedef __signed__ short __s16;
> +typedef unsigned short __u16;
> +typedef __signed__ int __s32;
> +typedef unsigned int __u32;
> +typedef __signed__ long long  __s64;
> +typedef unsigned long long  __u64;
> +typedef __u16 __le16;
> +typedef __u16 __be16;
> +typedef __u32 __le32;
> +typedef __u32 __be32;
> +typedef __u64 __le64;
> +typedef __u64 __be64;
> +#endif /* _WIN32 */
> +
>  typedef uint8_t u8;
>  typedef uint16_t u16;
>  typedef uint32_t u32;

Could you put most of the Windows compatibility definitions in a single new
header so that they don't clutter things up too much?

Including for things you put in other places, like O_BINARY.

> diff --git a/lib/enable.c b/lib/enable.c
> index 2478077..b49ba26 100644
> --- a/lib/enable.c
> +++ b/lib/enable.c
> @@ -11,14 +11,10 @@
>  
>  #include "lib_private.h"
>  
> +#ifndef _WIN32
> +

Could you just have the Makefile exclude files from the build instead?

	lib/enable.c
	programs/cmd_measure.c
	programs/cmd_enable.c

Then in programs/fsverity.c, ifdef out the 'measure' and 'enable' commands in
fsverity_commands[].

I think that would be easier.  Plus users won't get weird errors if they try to
use unsupported commands on Windows; the commands just won't be available.

> +#ifndef _WIN32
>  		if (asprintf(&msg2, "%s: %s", msg,
>  			     strerror_r(err, errbuf, sizeof(errbuf))) < 0)
> +#else
> +		strerror_s(errbuf, sizeof(errbuf), err);
> +		if (asprintf(&msg2, "%s: %s", msg, errbuf) < 0)
> +#endif
>  			goto out2;

Instead of doing this, could you provide a strerror_r() implementation in
programs/utils.c for _WIN32?

- Eric

Re: [fsverity-utils PATCH 2/2] Allow to build and run sign/digest on Windows

[Eric Biggers]

On Wed, Dec 16, 2020 at 11:08:51AM -0800, Eric Biggers wrote:
> > +#ifndef _WIN32
> >  		if (asprintf(&msg2, "%s: %s", msg,
> >  			     strerror_r(err, errbuf, sizeof(errbuf))) < 0)
> > +#else
> > +		strerror_s(errbuf, sizeof(errbuf), err);
> > +		if (asprintf(&msg2, "%s: %s", msg, errbuf) < 0)
> > +#endif
> >  			goto out2;
> 
> Instead of doing this, could you provide a strerror_r() implementation in
> programs/utils.c for _WIN32?
> 

Actually it would be lib/utils.c, not programs/utils.c.

- Eric

Re: [fsverity-utils PATCH 1/2] Remove unneeded includes

[Luca Boccassi]

[-- Attachment #1: Type: text/plain, Size: 2162 bytes --]

On Wed, 2020-12-16 at 10:44 -0800, Eric Biggers wrote:
> On Wed, Dec 16, 2020 at 05:27:18PM +0000, luca.boccassi@gmail.com wrote:
> > From: Luca Boccassi <luca.boccassi@microsoft.com>
> > 
> > Signed-off-by: Luca Boccassi <luca.boccassi@microsoft.com>
> > ---
> >  common/fsverity_uapi.h | 1 -
> >  programs/cmd_enable.c  | 1 -
> >  2 files changed, 2 deletions(-)
> > 
> > diff --git a/common/fsverity_uapi.h b/common/fsverity_uapi.h
> > index 33f4415..0006c35 100644
> > --- a/common/fsverity_uapi.h
> > +++ b/common/fsverity_uapi.h
> > @@ -10,7 +10,6 @@
> >  #ifndef _UAPI_LINUX_FSVERITY_H
> >  #define _UAPI_LINUX_FSVERITY_H
> >  
> > -#include <linux/ioctl.h>
> >  #include <linux/types.h>
> 
> fsverity_uapi.h is supposed to be kept in sync with
> include/uapi/linux/fsverity.h in the kernel source tree.
> 
> There's a reason why it includes <linux/ioctl.h>.  <linux/ioctl.h> provides the
> _IOW() and _IOWR() macros to expand the values of FS_IOC_ENABLE_VERITY and
> FS_IOC_MEASURE_VERITY.  Usually people referring to FS_IOC_* will include
> <sys/ioctl.h> in order to actually call ioctl() too.  However it's not
> guaranteed, so technically the header needs to include <linux/ioctl.h>.
> 
> Wrapping this include with '#ifdef _WIN32' would be better than removing it, as
> then it would be clear that it's a Windows-specific modification.
> 
> However I think an even better approach would be to add empty files
> win32-headers/linux/{types,ioctl.h} and add -Iwin32-headers to CPPFLAGS, so that
> these headers can still be included in the Windows build without having to
> modify the source files.

I took the first approach in v2, as it's a bit simpler. If you feel
strongly about it I can do the latter.

> > diff --git a/programs/cmd_enable.c b/programs/cmd_enable.c
> > index fdf26c7..14c3c17 100644
> > --- a/programs/cmd_enable.c
> > +++ b/programs/cmd_enable.c
> > @@ -14,7 +14,6 @@
> >  #include <fcntl.h>
> >  #include <getopt.h>
> >  #include <limits.h>
> > -#include <sys/ioctl.h>
> >  
> 
> This part looks fine though, as cmd_enable.c no longer directly uses an ioctl.
> 
> - Eric


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [fsverity-utils PATCH 2/2] Allow to build and run sign/digest on Windows

[Luca Boccassi]

[-- Attachment #1: Type: text/plain, Size: 8524 bytes --]

On Wed, 2020-12-16 at 11:08 -0800, Eric Biggers wrote:
> On Wed, Dec 16, 2020 at 05:27:19PM +0000, luca.boccassi@gmail.com wrote:
> > From: Luca Boccassi <luca.boccassi@microsoft.com>
> > 
> > Add some minimal compat type defs, and stub out the enable/measure
> > functions. Also add a way to handle the fact that mingw adds a
> > .exe extension automatically in the Makefile install rules.
> > 
> > Signed-off-by: Luca Boccassi <luca.boccassi@microsoft.com>
> > ---
> > So this is proably going to look strange, and believe me the feeling is shared.
> > It's actually the first time _ever_ I had to compile and run something on
> > Windows, which is ironic in itself - the O_BINARY stuff is probably WIN32-101 and
> > it took me an hour to find out.
> > Anyway, I've got some groups building their payloads on Windows, so we need to
> > provide native tooling. Among these are fsverity tools to get the digest and
> > sign files.
> > This patch stubs out and returns EOPNOTSUPP from the measure/enable functions,
> > since they are linux-host only, and adds some (hopefully) minimal and unintrusive
> > compat ifdefs for the rest. There's also a change in the makefile, since the
> > build toolchain (yocto + mingw) for some reason automatically names executables
> > as .exe. Biggest chunk is the types definitions I guess. The ugliest is the
> > print stuff.
> > 
> > Note that with this I do not ask you in any way, shape or form to be responsible
> > for the correct functioning or even compiling on WIN32 of these utilities - if
> > anything ever breaks, we'll find out and take care of it. I could keep all of this
> > out of tree, but I figured I'd try to see if you are amenable to merge at least
> > some part of it.
> > 
> > I've tested that both Linux and WIN32 builds of digest and sign commands return
> > the exact same output for the same input.
> 
> On Linux, can you make it easily cross-compile for Windows using
> 'make CC=x86_64-w64-mingw32-gcc'?  That would be ideal, so that that command can
> be added to scripts/run-tests.sh, so that I can make sure it stays building.
> 
> I probably won't actually test *running* it on Windows, as that would be more
> work.  But building should be easy.

Yes, that's how I've been compiling it - with the addition to find the
openssl library and headers, as it seems on Debian mingw32-gcc doesn't
define any system paths. Ie:

make CC=x86_64-w64-mingw32-gcc-8.3-win32 CPPFLAGS="-I /tmp/win" LDFLAGS="-L/tmp/win"

If libcrypto.dll and include/openssl are visible to mingw32-gcc out of
the box, then CC is the only thing you should need.

> > diff --git a/Makefile b/Makefile
> > index bfe83c4..fe89e18 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -63,6 +63,7 @@ INCDIR          ?= $(PREFIX)/include
> >  LIBDIR          ?= $(PREFIX)/lib
> >  DESTDIR         ?=
> >  PKGCONF         ?= pkg-config
> > +EXEEXT          ?=
> 
> It looks like you're requiring the caller to manually specify EXEEXT.  You could
> use something like the following to automatically detect when CC is MinGW and
> set EXEEXT and AR appropriately:
> 
> # Compiling for Windows with MinGW?
> ifneq ($(findstring -mingw,$(shell $(CC) -dumpmachine 2>/dev/null)),)
> 	EXEEXT := .exe
> 	# Derive $(AR) from $(CC).
> 	AR := $(shell echo $(CC) | \
>                 sed -E 's/g?cc(-?[0-9]+(\.[0-9]+)*)?(\.exe)?$$/ar\3/')
> endif

Done in v2, without overriding AR - it seems to work as-is.

> >  # Rebuild if a user-specified setting that affects the build changed.
> >  .build-config: FORCE
> > @@ -87,9 +88,9 @@ CFLAGS          += $(shell "$(PKGCONF)" libcrypto --cflags 2>/dev/null || echo)
> >  # If we are dynamically linking, when running tests we need to override
> >  # LD_LIBRARY_PATH as no RPATH is set
> >  ifdef USE_SHARED_LIB
> > -RUN_FSVERITY    = LD_LIBRARY_PATH=./ ./fsverity
> > +RUN_FSVERITY    = LD_LIBRARY_PATH=./ ./fsverity$(EXEEXT)
> >  else
> > -RUN_FSVERITY    = ./fsverity
> > +RUN_FSVERITY    = ./fsverity$(EXEEXT)
> >  endif
> >  
> >  ##############################################################################
> > @@ -186,7 +187,7 @@ test_programs:$(TEST_PROGRAMS)
> >  # want to run the full tests.
> >  check:fsverity test_programs
> >  	for prog in $(TEST_PROGRAMS); do \
> > -		$(TEST_WRAPPER_PROG) ./$$prog || exit 1; \
> > +		$(TEST_WRAPPER_PROG) ./$$prog$(EXEEXT) || exit 1; \
> >  	done
> >  	$(RUN_FSVERITY) --help > /dev/null
> >  	$(RUN_FSVERITY) --version > /dev/null
> > @@ -202,7 +203,7 @@ check:fsverity test_programs
> >  
> >  install:all
> >  	install -d $(DESTDIR)$(LIBDIR)/pkgconfig $(DESTDIR)$(INCDIR) $(DESTDIR)$(BINDIR)
> > -	install -m755 fsverity $(DESTDIR)$(BINDIR)
> > +	install -m755 fsverity$(EXEEXT) $(DESTDIR)$(BINDIR)
> >  	install -m644 libfsverity.a $(DESTDIR)$(LIBDIR)
> >  	install -m755 libfsverity.so.$(SOVERSION) $(DESTDIR)$(LIBDIR)
> >  	ln -sf libfsverity.so.$(SOVERSION) $(DESTDIR)$(LIBDIR)/libfsverity.so
> > @@ -215,7 +216,7 @@ install:all
> >  	chmod 644 $(DESTDIR)$(LIBDIR)/pkgconfig/libfsverity.pc
> >  
> >  uninstall:
> > -	rm -f $(DESTDIR)$(BINDIR)/fsverity
> > +	rm -f $(DESTDIR)$(BINDIR)/fsverity$(EXEEXT)
> >  	rm -f $(DESTDIR)$(LIBDIR)/libfsverity.a
> >  	rm -f $(DESTDIR)$(LIBDIR)/libfsverity.so.$(SOVERSION)
> >  	rm -f $(DESTDIR)$(LIBDIR)/libfsverity.so
> > @@ -232,4 +233,4 @@ help:
> >  
> >  clean:
> >  	rm -f $(DEFAULT_TARGETS) $(TEST_PROGRAMS) \
> > -		lib/*.o programs/*.o .build-config fsverity.sig
> > +		fsverity$(EXEEXT) lib/*.o programs/*.o .build-config fsverity.sig
> 
> Do you need libfsverity to be built properly for Windows (producing .dll, .lib,
> and .def files), or are you just looking to build the fsverity binary?  At the
> moment you're just doing the latter.  There are a bunch of differences between
> libraries on Windows and Linux, so hopefully you don't need the library built
> properly for Windows, but it would be possible.

I do not need the library at the moment no, I just need the binary.

> > diff --git a/common/common_defs.h b/common/common_defs.h
> > index 279385a..a869532 100644
> > --- a/common/common_defs.h
> > +++ b/common/common_defs.h
> > @@ -15,6 +15,30 @@
> >  #include <stddef.h>
> >  #include <stdint.h>
> >  
> > +#ifdef _WIN32
> > +/* Some minimal definitions to allow the digest/sign commands to run under Windows */
> > +#  ifndef ENOPKG
> > +#    define ENOPKG 65
> > +#  endif
> > +#  ifndef __cold
> > +#    define __cold
> > +#  endif
> > +typedef __signed__ char __s8;
> > +typedef unsigned char __u8;
> > +typedef __signed__ short __s16;
> > +typedef unsigned short __u16;
> > +typedef __signed__ int __s32;
> > +typedef unsigned int __u32;
> > +typedef __signed__ long long  __s64;
> > +typedef unsigned long long  __u64;
> > +typedef __u16 __le16;
> > +typedef __u16 __be16;
> > +typedef __u32 __le32;
> > +typedef __u32 __be32;
> > +typedef __u64 __le64;
> > +typedef __u64 __be64;
> > +#endif /* _WIN32 */
> > +
> >  typedef uint8_t u8;
> >  typedef uint16_t u16;
> >  typedef uint32_t u32;
> 
> Could you put most of the Windows compatibility definitions in a single new
> header so that they don't clutter things up too much?
> 
> Including for things you put in other places, like O_BINARY.

Added common/win32_defs.h in v2

> > diff --git a/lib/enable.c b/lib/enable.c
> > index 2478077..b49ba26 100644
> > --- a/lib/enable.c
> > +++ b/lib/enable.c
> > @@ -11,14 +11,10 @@
> >  
> >  #include "lib_private.h"
> >  
> > +#ifndef _WIN32
> > +
> 
> Could you just have the Makefile exclude files from the build instead?
> 
> 	lib/enable.c
> 	programs/cmd_measure.c
> 	programs/cmd_enable.c
> 
> Then in programs/fsverity.c, ifdef out the 'measure' and 'enable' commands in
> fsverity_commands[].
> 
> I think that would be easier.  Plus users won't get weird errors if they try to
> use unsupported commands on Windows; the commands just won't be available.

Done in v2

> > +#ifndef _WIN32
> >  		if (asprintf(&msg2, "%s: %s", msg,
> >  			     strerror_r(err, errbuf, sizeof(errbuf))) < 0)
> > +#else
> > +		strerror_s(errbuf, sizeof(errbuf), err);
> > +		if (asprintf(&msg2, "%s: %s", msg, errbuf) < 0)
> > +#endif
> >  			goto out2;
> 
> Instead of doing this, could you provide a strerror_r() implementation in
> programs/utils.c for _WIN32?

Added in v2.

Thanks for the review!

-- 
Kind regards,
Luca Boccassi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]