Re: [Minios-devel] [UNIKRAFT PATCH v4 2/4] lib/uksp: Introduce uksp library

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 04.02.20 15:09, Vlad-Andrei BĂDOIU (78692) wrote:
> From: Vlad-Andrei BĂDOIU (78692) <vlad_andrei.badoiu@xxxxxxxxxxxxxxx>
>
> This library provides the necessary functionalities for the stack
> protector.
>
> A make clean is required when toggling the stack smashing protection
> option.
>
> Signed-off-by: Vlad-Andrei Badoiu <vlad_andrei.badoiu@xxxxxxxxxxxxxxx>
> ---
>   lib/Makefile.uk          |  1 +
>   lib/uksp/Config.uk       | 52 +++++++++++++++++++++++++++++++++++
>   lib/uksp/Makefile.uk     |  5 ++++
>   lib/uksp/exportsyms.uk   |  2 ++
>   lib/uksp/include/uk/sp.h | 58 ++++++++++++++++++++++++++++++++++++++++
>   lib/uksp/ssp.c           | 50 ++++++++++++++++++++++++++++++++++
>   6 files changed, 168 insertions(+)
>   create mode 100644 lib/uksp/Config.uk
>   create mode 100644 lib/uksp/Makefile.uk
>   create mode 100644 lib/uksp/exportsyms.uk
>   create mode 100644 lib/uksp/include/uk/sp.h
>   create mode 100644 lib/uksp/ssp.c
>
> diff --git a/lib/Makefile.uk b/lib/Makefile.uk
> index aa7e7302..c02a3c50 100644
> --- a/lib/Makefile.uk
> +++ b/lib/Makefile.uk
> @@ -34,3 +34,4 @@ $(eval $(call _import_lib,$(CONFIG_UK_BASE)/lib/uktime))
>   $(eval $(call _import_lib,$(CONFIG_UK_BASE)/lib/ukmmap))
>   $(eval $(call _import_lib,$(CONFIG_UK_BASE)/lib/ukblkdev))
>   $(eval $(call _import_lib,$(CONFIG_UK_BASE)/lib/posix-process))
> +$(eval $(call _import_lib,$(CONFIG_UK_BASE)/lib/uksp))
> diff --git a/lib/uksp/Config.uk b/lib/uksp/Config.uk
> new file mode 100644
> index 00000000..2ec953d4
> --- /dev/null
> +++ b/lib/uksp/Config.uk
> @@ -0,0 +1,52 @@
> +config LIBUKSP
> +       bool "uksp: Stack protector"
> +       select HAVE_STACKPROTECTOR
> +       select LIBUKSCHED
You can remove the dependency to LIBUKSCHED. See my comments below.

> +       default n
> +
> +if LIBUKSP
> +choice
> +       prompt "Stack protector level"
> +       default STACKPROTECTOR_NONE
> +       help
> +         Set the stack protector level
> +
> +config STACKPROTECTOR_NONE
> +       bool "None"
> +       help
> +               Do not use stack protector, use -fno-stack-protector.
I think STACKPROTECTOR_NONE can be removed from the library menu. As 
soon as you remove uksp from the build, the build system should do this 
already.
> +
> +config STACKPROTECTOR_REGULAR
> +       bool "Regular"
> +       help
> +               Regular stack protector, use -fstack-protector.
> +
> +config STACKPROTECTOR_STRONG
> +       bool "Strong"
> +       help
> +               Strong stack protector, use -fstack-protector-strong.
> +
> +config STACKPROTECTOR_ALL
> +       bool "All"
> +       help
> +               Protect all functions, use -fstack-protector-all.
> +endchoice
> +
> +choice
> +       prompt "Canary Value"
> +       default LIBUKSP_VALUE_CONSTANT
> +
> +config LIBUKSP_VALUE_USECONSTANT
> +       bool "Compiled-in constant"
> +
> +config LIBUKSP_VALUE_RANDOM
> +       bool "Random variable"
> +       select LIBUKSWRAND
> +endchoice
> +
> +config LIBUKSP_VALUE_CONSTANT
> +       int "Canary value"
> +       depends on LIBUKSP_VALUE_USECONSTANT
> +       default 42
> +
> +endif
> diff --git a/lib/uksp/Makefile.uk b/lib/uksp/Makefile.uk
> new file mode 100644
> index 00000000..6c391c9d
> --- /dev/null
> +++ b/lib/uksp/Makefile.uk
> @@ -0,0 +1,5 @@
> +$(eval $(call addlib_s,libuksp,$(CONFIG_LIBUKSP)))
> +
> +CINCLUDES-y += -I$(LIBUKSP_BASE)/include
> +
> +LIBUKSP_SRCS-y += $(LIBUKSP_BASE)/ssp.c
Can you include here setting the COMPFLAGS from the next patch? I think 
the next patch should actually be before this one.
> diff --git a/lib/uksp/exportsyms.uk b/lib/uksp/exportsyms.uk
> new file mode 100644
> index 00000000..fbc319e7
> --- /dev/null
> +++ b/lib/uksp/exportsyms.uk
> @@ -0,0 +1,2 @@
> +__stack_chk_fail
> +__stack_chk_guard
> diff --git a/lib/uksp/include/uk/sp.h b/lib/uksp/include/uk/sp.h
> new file mode 100644
> index 00000000..33a6b6f2
> --- /dev/null
> +++ b/lib/uksp/include/uk/sp.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Authors: Vlad-Andrei Badoiu <vlad_andrei.badoiu@xxxxxxxxxxxxxxx>
> + *
> + * Copyright (c) 2019, University Politehnica of Bucharest. All rights 
> reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the copyright holder nor the names of its
> + *    contributors may be used to endorse or promote products derived from
> + *    this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + *
> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
Please check that your headers do not introduce this extra sentence to 
the license. We recently figured that we got this line somewhere and 
somehow in and copied it to many places. It is for sure incompatible 
with the BSD license ahead.
> + */
> +
> +#ifndef __UK_STACKPROTECTOR_H__
> +#define __UK_STACKPROTECTOR_H__
> +
> +#include <uk/swrand.h>
> +#include <uk/config.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +extern unsigned long __stack_chk_guard;
Should we declare it as const? We could use a deconst trick for the 
initialization.
> +
> +#ifdef CONFIG_LIBUKSP_VALUE_RANDOM
> +#define INIT_STACK_CANARY() (__stack_chk_guard = uk_swrand_randr())
> +#endif
> +#ifdef CONFIG_LIBUKSP_VALUE_USECONSTANT
> +#define INIT_STACK_CANARY() (__stack_chk_guard = CONFIG_LIBUKSP_VALUE_CONSTANT)
> +#endif
In the case of a pre-compiled value, we should initialize the value 
within the C file. INIT_STACK_CANARY() could be resolved to an empty 
macro in this case.
Because of name spacing, I would suggest to call this macro
UKSP_INIT_CANARY() instead... are you okay with it?

> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* __UK_STACKPROTECTOR_H__ */
> diff --git a/lib/uksp/ssp.c b/lib/uksp/ssp.c
> new file mode 100644
> index 00000000..8be3a051
> --- /dev/null
> +++ b/lib/uksp/ssp.c
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Authors: Badoiu Vlad-Andrei <vlad_andrei.badoiu@xxxxxxxxxxxxxxx>
> + *
> + * Copyright (c) 2019, University Politehnica of Bucharest. All rights 
> reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the copyright holder nor the names of its
> + *    contributors may be used to endorse or promote products derived from
> + *    this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + *
> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> + */
> +
> +#include <uk/assert.h>
> +#include <uk/swrand.h>
> +#include <uk/config.h>
> +#include <uk/ctors.h>
> +#include <uk/thread.h>
> +
> +unsigned long __stack_chk_guard;
I would do:

#ifdef CONFIG_LIBUKSP_VALUE_USECONSTANT
unsigned long __stack_chk_guard = CONFIG_LIBUKSP_VALUE_CONSTANT
#else
unsigned long __stack_chk_guard = 0xDEADBEEF
#endif

You could try to declare it as const, too. At least the compiler would 
then guard any change of the value.
> +
> +__attribute__((noreturn))
> +void __stack_chk_fail(void)
> +{
> +       struct uk_thread *current_thread;
> +
> +       current_thread = uk_thread_current();
uk_thread_current()->ctx is not the stack pointer, it is the reference 
to the current platform context struct. Additionally, 
uk_thread_current() is only available with uksched. Use ukarch_read_sp() 
instead, it returns the current stack pointer and you are independent of 
scheduling support.
> +       UK_CRASH("Stack smashing detected. SP %p\n", current_thread->ctx);
> +}
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel