| Message ID | 1536163596-5310-2-git-send-email-angelo@amarulasolutions.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add tainting support to buildroot | expand |
Hello, +Yann/Arnout in Cc. On Wed, 5 Sep 2018 18:06:34 +0200, Angelo Compagnucci wrote: > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index 91b61c6..bcb4acd 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -551,6 +551,10 @@ ifndef $(2)_REDISTRIBUTE > endif > endif > > +ifdef $(2)_TAINTS > +BR2_TAINTED_BY+=$$($(2)_RAWNAME) > +endif I was almost going to apply and push this, but then I found a fairly major problem. With your implementation, as soon as a package sets <pkg>_TAINTS = YES, it will be part of the BR2_TAINTED_BY list, regardless of whether the package is enabled or not. It happens to work OK with nodejs, because the NODEJS_TAINTS = YES is inside a condition that NODEJS_MODULES_LIST != "". However, if you put NODEJS_TAINTS = YES outside of that condition in nodejs.mk, you will see that the build is always tainted by nodejs, regardless of whether nodejs is enabled or not. Not good. This can easily be fixed by moving the BR2_TAINTED_BY += line inside the: ifeq ($$($$($(2)_KCONFIG_VAR)),y) condition, which is true only when the package is really enabled. However, this only works for target packages. What about host packages ? Can they taint the build ? If so, how do we handle that ? When I was about to apply, I had added something like this in pkg-generic.mk: ifndef $(2)_TAINTS ifdef $(3)_TAINTS $(2)_TAINTS = $$($(3)_TAINTS) endif endif so that HOST_<pkg>_TAINTS is automatically defined to the same value as <pkg>_TAINTS. However, we have no way to know if a host package is enabled or not, since most host packages don't have any corresponding Config.in option. So: do we care about host packages for "tainting" ? If we do care, how do we handle this ? Best regards, Thomas
Hi Thomas, On Wed, Sep 5, 2018 at 9:37 PM, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > Hello, > > +Yann/Arnout in Cc. > > On Wed, 5 Sep 2018 18:06:34 +0200, Angelo Compagnucci wrote: > >> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk >> index 91b61c6..bcb4acd 100644 >> --- a/package/pkg-generic.mk >> +++ b/package/pkg-generic.mk >> @@ -551,6 +551,10 @@ ifndef $(2)_REDISTRIBUTE >> endif >> endif >> >> +ifdef $(2)_TAINTS >> +BR2_TAINTED_BY+=$$($(2)_RAWNAME) >> +endif > > I was almost going to apply and push this, but then I found a fairly > major problem. With your implementation, as soon as a package sets > <pkg>_TAINTS = YES, it will be part of the BR2_TAINTED_BY list, > regardless of whether the package is enabled or not. > > It happens to work OK with nodejs, because the NODEJS_TAINTS = YES is > inside a condition that NODEJS_MODULES_LIST != "". However, if you put > NODEJS_TAINTS = YES outside of that condition in nodejs.mk, you will > see that the build is always tainted by nodejs, regardless of whether > nodejs is enabled or not. Not good. Thank you so much for finding this. > This can easily be fixed by moving the BR2_TAINTED_BY += line inside > the: > > ifeq ($$($$($(2)_KCONFIG_VAR)),y) > > condition, which is true only when the package is really enabled. Yes, it works like a charm! > However, this only works for target packages. What about host > packages ? Can they taint the build ? If so, how do we handle that ? > > When I was about to apply, I had added something like this in > pkg-generic.mk: > > ifndef $(2)_TAINTS > ifdef $(3)_TAINTS > $(2)_TAINTS = $$($(3)_TAINTS) > endif > endif > > so that HOST_<pkg>_TAINTS is automatically defined to the same value as > <pkg>_TAINTS. However, we have no way to know if a host package is > enabled or not, since most host packages don't have any > corresponding Config.in option. > > So: do we care about host packages for "tainting" ? If we do care, how > do we handle this ? I think we can demand the decision to when we will have a host package that needs tainting support. Moreover, I don't think the demand for host packages that needs tainting support will be high in the future. I'll send an updated patch. > > Best regards, > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Hello, On Wed, 5 Sep 2018 23:45:47 +0200, Angelo Compagnucci wrote: > > So: do we care about host packages for "tainting" ? If we do care, how > > do we handle this ? > > I think we can demand the decision to when we will have a host package > that needs tainting support. > > Moreover, I don't think the demand for host packages that needs > tainting support will be high in the future. Then we need to make that explicit: ifeq ($$($(2)_TYPE),host) ifneq ($$($(2)_TAINTS),) $$(error "Host package $(1) has $(2)_TAINTS set: not supported) endif endif of course, please check that this really works as expected. Best regards, Thomas
Hi Thomas, On Thu, Sep 6, 2018 at 12:01 AM, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > Hello, > > On Wed, 5 Sep 2018 23:45:47 +0200, Angelo Compagnucci wrote: > >> > So: do we care about host packages for "tainting" ? If we do care, how >> > do we handle this ? >> >> I think we can demand the decision to when we will have a host package >> that needs tainting support. >> >> Moreover, I don't think the demand for host packages that needs >> tainting support will be high in the future. > > Then we need to make that explicit: > > ifeq ($$($(2)_TYPE),host) > ifneq ($$($(2)_TAINTS),) > $$(error "Host package $(1) has $(2)_TAINTS set: not supported) > endif > endif > > of course, please check that this really works as expected. Done! > > Best regards, > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
diff --git a/Makefile b/Makefile index 9d66bba..ad61130 100644 --- a/Makefile +++ b/Makefile @@ -758,12 +758,21 @@ endif touch $(TARGET_DIR)/usr +.PHONY: check-tainted +check-tainted: +ifneq ($(BR2_TAINTED_BY),) + $(error Your buildroot configuration is tainted by: $(BR2_TAINTED_BY)) +else + @echo "Your buildroot configuration is not tainted" +endif + .PHONY: target-post-image target-post-image: $(TARGETS_ROOTFS) target-finalize @rm -f $(ROOTFS_COMMON_TAR) @$(foreach s, $(call qstrip,$(BR2_ROOTFS_POST_IMAGE_SCRIPT)), \ $(call MESSAGE,"Executing post-image script $(s)"); \ $(EXTRA_ENV) $(s) $(BINARIES_DIR) $(call qstrip,$(BR2_ROOTFS_POST_SCRIPT_ARGS))$(sep)) + $(if $(BR2_TAINTED_BY),@echo "WARNING: Your buildroot configuration is tainted by: $(BR2_TAINTED_BY).") .PHONY: source source: $(foreach p,$(PACKAGES),$(p)-all-source) @@ -1070,6 +1079,7 @@ help: @echo ' source - download all sources needed for offline-build' @echo ' external-deps - list external packages used' @echo ' legal-info - generate info about license compliance' + @echo ' check-tainted - check if any selected package harms build reproducibility or licensing' @echo ' printvars - dump all the internal variables' @echo @echo ' make V=0|1 - 0 => quiet build (default), 1 => verbose build' diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index 91b61c6..bcb4acd 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -551,6 +551,10 @@ ifndef $(2)_REDISTRIBUTE endif endif +ifdef $(2)_TAINTS +BR2_TAINTED_BY+=$$($(2)_RAWNAME) +endif + $(2)_REDISTRIBUTE ?= YES $(2)_REDIST_SOURCES_DIR = $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))/$$($(2)_BASENAME_RAW) @@ -909,6 +913,10 @@ else $(Q)$$(foreach F,$$($(2)_LICENSE_FILES),$$(call legal-license-file,$$($(2)_RAWNAME),$$($(2)_BASENAME_RAW),$$($(2)_PKGDIR),$$(F),$$($(2)_DIR)/$$(F),$$(call UPPERCASE,$(4)))$$(sep)) endif # license files +ifeq ($$(call qstrip,$$($(2)_TAINTS)),YES) + $(Q)$$(call legal-warning-pkg,$$($(2)_RAWNAME),unknown license for additional modules or dependencies) +endif + ifeq ($$($(2)_SITE_METHOD),local) # Packages without a tarball: don't save and warn @$$(call legal-warning-nosource,$$($(2)_RAWNAME),local)