Skip to content

Add missing system calls for ADC driver subsystem #15001

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 30, 2019
Merged

Add missing system calls for ADC driver subsystem #15001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b2a5558
userspace: add const qualifiers to user copy fns
Mar 28, 2019
d657a8c
userspace: set default priv stack size to 1024
Mar 28, 2019
14c5054
drivers: adc: add some missing system calls
Mar 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions arch/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,10 +178,7 @@ config USERSPACE

config PRIVILEGED_STACK_SIZE
int "Size of privileged stack"
default 512 if MPU_STACK_GUARD || BUILTIN_STACK_GUARD
default 512 if COVERAGE_GCOV
default 384 if ARC
default 256
default 1024
Copy link
Member

@ioannisg ioannisg Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the rationale, here, but, I see that the priv stack starts getting too big

Copy link
Contributor Author

@andrewboie andrewboie Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what you mean by too big
The actual minimum value for this, that satisfies maximum depth for all kernel/driver syscalls is unknown at this time.

depends on ARCH_HAS_USERSPACE
help
This option sets the privileged stack region size that will be used
Expand Down
1 change: 1 addition & 0 deletions drivers/adc/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ zephyr_library_sources_ifdef(CONFIG_ADC_NRFX_ADC adc_nrfx_adc.c)
zephyr_library_sources_ifdef(CONFIG_ADC_NRFX_SAADC adc_nrfx_saadc.c)
zephyr_library_sources_ifdef(CONFIG_ADC_INTEL_QUARK_SE_C1000_SS adc_intel_quark_se_c1000_ss.c)
zephyr_library_sources_ifdef(CONFIG_ADC_INTEL_QUARK_D2000 adc_intel_quark_d2000.c)
zephyr_library_sources_ifdef(CONFIG_USERSPACE adc_handlers.c)
64 changes: 64 additions & 0 deletions drivers/adc/adc_handlers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Copyright (c) 2019 Intel Corporation
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <adc.h>
#include <syscall_handler.h>
#include <kernel.h>

Z_SYSCALL_HANDLER(adc_channel_setup, dev, user_channel_cfg)
{
struct adc_channel_cfg channel_cfg;

Z_OOPS(Z_SYSCALL_DRIVER_ADC(dev, channel_setup));
Z_OOPS(z_user_from_copy(&channel_cfg,
(struct adc_channel_cfg *)user_channel_cfg,
sizeof(struct adc_channel_cfg)));

return z_impl_adc_channel_setup((struct device *)dev, &channel_cfg);
}

static bool copy_sequence(struct adc_sequence *dst,
struct adc_sequence_options *options,
struct adc_sequence *src)
{
if (z_user_from_copy(dst, src, sizeof(struct adc_sequence)) != 0) {
printk("couldn't copy adc_sequence struct\n");
return false;
}

if (dst->options) {
if (z_user_from_copy(options, dst->options,
Copy link
Member

@ceolin ceolin Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't copy from src->options ?

Copy link
Contributor Author

@andrewboie andrewboie Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at this point in the code, src and dst have the same contents since we just previously copied src (a pointer into user memory) into dst (a pointer on the supervisor mode call stack)

Copy link
Member

@ceolin ceolin Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not making sense to me though. Why do you need this check and copy then ? dst is an uninitialised variable:


Z_SYSCALL_HANDLER(adc_read, dev, user_sequence)
{
	struct adc_sequence sequence;
	struct adc_sequence_options options;

	Z_OOPS(Z_SYSCALL_DRIVER_ADC(dev, read));
	Z_OOPS(Z_SYSCALL_VERIFY_MSG(copy_sequence(&sequence, &options,
					(struct adc_sequence *)user_sequence),
				    "invalid ADC sequence"));
	if (sequence.options != NULL) {
		Z_OOPS(Z_SYSCALL_VERIFY_MSG(sequence.options->callback == NULL,
			    "ADC sequence callbacks forbidden from user mode"));
	}

	return z_impl_adc_read((struct device *)dev, &sequence);
}

Copy link
Contributor Author

@andrewboie andrewboie Mar 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to copy user_sequence (lives somehere in user memory) into kernel memory (sequence) in order to prevent TOCTOU issues. notice I am not passing user_sequence to the syscall implementation.

Copy link
Member

@ceolin ceolin Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, but this code is not copying user sequence, it is copying sequence that was allocate in this function stack and was not initialized. Well, it seems that I'm not understanding it :/

Copy link
Contributor Author

@andrewboie andrewboie Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're not reading the code right, you have it backwards. user_sequence=src sequence=dst

Copy link
Member

@ceolin ceolin Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is the problem, in copy_sequence you are not using user_sequence=src but sequece=dst

static bool copy_sequence(struct adc_sequence *dst,
			  struct adc_sequence_options *options,
			  struct adc_sequence *src)
{
...

	if (dst->options) { /* dst == sequence that is no uninitialised */
		if (z_user_from_copy(options, dst->options,
				sizeof(struct adc_sequence_options)) != 0) {
			printk("couldn't copy adc_options struct\n");
			return false;
		}
		dst->options = options;
	}
...
}

Copy link
Contributor Author

@andrewboie andrewboie Mar 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please post the whole function.

You will see that immediately before the check to dst->options, all of src got copied into dst. It is initialized.

dst->options points to a different memory buffer provided by the user, we copy it and then update dst->options.

Copy link
Member

@ceolin ceolin Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that first copy, damn it. My bad, sorry for all noise

sizeof(struct adc_sequence_options)) != 0) {
printk("couldn't copy adc_options struct\n");
return false;
}
dst->options = options;
}

if (Z_SYSCALL_MEMORY_WRITE(dst->buffer, dst->buffer_size) != 0) {
printk("no access to buffer memory\n");
return false;
}
return true;
}


Z_SYSCALL_HANDLER(adc_read, dev, user_sequence)
{
struct adc_sequence sequence;
struct adc_sequence_options options;

Z_OOPS(Z_SYSCALL_DRIVER_ADC(dev, read));
Z_OOPS(Z_SYSCALL_VERIFY_MSG(copy_sequence(&sequence, &options,
(struct adc_sequence *)user_sequence),
"invalid ADC sequence"));
if (sequence.options != NULL) {
Z_OOPS(Z_SYSCALL_VERIFY_MSG(sequence.options->callback == NULL,
"ADC sequence callbacks forbidden from user mode"));
}

return z_impl_adc_read((struct device *)dev, &sequence);
}
14 changes: 11 additions & 3 deletions include/adc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,8 +306,11 @@ struct adc_driver_api {
* @retval 0 On success.
* @retval -EINVAL If a parameter with an invalid value has been provided.
*/
static inline int adc_channel_setup(struct device *dev,
const struct adc_channel_cfg *channel_cfg)
__syscall int adc_channel_setup(struct device *dev,
const struct adc_channel_cfg *channel_cfg);

static inline int z_impl_adc_channel_setup(struct device *dev,
const struct adc_channel_cfg *channel_cfg)
{
const struct adc_driver_api *api = dev->driver_api;

Expand All @@ -332,7 +335,10 @@ static inline int adc_channel_setup(struct device *dev,
* in the buffer, but at least some of them were taken with
* an extra delay compared to what was scheduled.
*/
static inline int adc_read(struct device *dev,
__syscall int adc_read(struct device *dev,
const struct adc_sequence *sequence);

static inline int z_impl_adc_read(struct device *dev,
const struct adc_sequence *sequence)
{
const struct adc_driver_api *api = dev->driver_api;
Expand Down Expand Up @@ -366,6 +372,8 @@ static inline int adc_read_async(struct device *dev,
}
#endif /* CONFIG_ADC_ASYNC */

#include <syscalls/adc.h>

/**
* @}
*/
Expand Down
10 changes: 5 additions & 5 deletions kernel/include/syscall_handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ static inline size_t z_user_string_nlen(const char *src, size_t maxlen,
* @return An allocated buffer with the data copied within it, or NULL
* if some error condition occurred
*/
extern void *z_user_alloc_from_copy(void *src, size_t size);
extern void *z_user_alloc_from_copy(const void *src, size_t size);

/**
* @brief Copy data from user mode
Expand All @@ -204,7 +204,7 @@ extern void *z_user_alloc_from_copy(void *src, size_t size);
* @retval 0 On success
* @retval EFAULT On memory access error
*/
extern int z_user_from_copy(void *dst, void *src, size_t size);
extern int z_user_from_copy(void *dst, const void *src, size_t size);

/**
* @brief Copy data to user mode
Expand All @@ -219,7 +219,7 @@ extern int z_user_from_copy(void *dst, void *src, size_t size);
* @retval 0 On success
* @retval EFAULT On memory access error
*/
extern int z_user_to_copy(void *dst, void *src, size_t size);
extern int z_user_to_copy(void *dst, const void *src, size_t size);

/**
* @brief Copy a C string from userspace into a resource pool allocation
Expand All @@ -235,7 +235,7 @@ extern int z_user_to_copy(void *dst, void *src, size_t size);
* @param maxlen Maximum size of the string including trailing NULL
* @return The duplicated string, or NULL if an error occurred.
*/
extern char *z_user_string_alloc_copy(char *src, size_t maxlen);
extern char *z_user_string_alloc_copy(const char *src, size_t maxlen);

/**
* @brief Copy a C string from userspace into a provided buffer
Expand All @@ -253,7 +253,7 @@ extern char *z_user_string_alloc_copy(char *src, size_t maxlen);
* to maxlen
* @retval EFAULT On memory access error
*/
extern int z_user_string_copy(char *dst, char *src, size_t maxlen);
extern int z_user_string_copy(char *dst, const char *src, size_t maxlen);

#define Z_OOPS(expr) \
do { \
Expand Down
12 changes: 6 additions & 6 deletions kernel/userspace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ void z_object_uninit(void *obj)
/*
* Copy to/from helper functions used in syscall handlers
*/
void *z_user_alloc_from_copy(void *src, size_t size)
void *z_user_alloc_from_copy(const void *src, size_t size)
{
void *dst = NULL;
k_spinlock_key_t key = k_spin_lock(&ucopy_lock);
Expand All @@ -654,7 +654,7 @@ void *z_user_alloc_from_copy(void *src, size_t size)
return dst;
}

static int user_copy(void *dst, void *src, size_t size, bool to_user)
static int user_copy(void *dst, const void *src, size_t size, bool to_user)
{
int ret = EFAULT;
k_spinlock_key_t key = k_spin_lock(&ucopy_lock);
Expand All @@ -672,17 +672,17 @@ static int user_copy(void *dst, void *src, size_t size, bool to_user)
return ret;
}

int z_user_from_copy(void *dst, void *src, size_t size)
int z_user_from_copy(void *dst, const void *src, size_t size)
{
return user_copy(dst, src, size, false);
}

int z_user_to_copy(void *dst, void *src, size_t size)
int z_user_to_copy(void *dst, const void *src, size_t size)
{
return user_copy(dst, src, size, true);
}

char *z_user_string_alloc_copy(char *src, size_t maxlen)
char *z_user_string_alloc_copy(const char *src, size_t maxlen)
{
unsigned long actual_len;
int err;
Expand All @@ -709,7 +709,7 @@ char *z_user_string_alloc_copy(char *src, size_t maxlen)
return ret;
}

int z_user_string_copy(char *dst, char *src, size_t maxlen)
int z_user_string_copy(char *dst, const char *src, size_t maxlen)
{
unsigned long actual_len;
int ret, err;
Expand Down
16 changes: 10 additions & 6 deletions tests/drivers/adc/adc_api/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,28 @@
* @}
*/


#include <zephyr.h>
#include <ztest.h>

extern void test_adc_sample_one_channel(void);
extern void test_adc_sample_two_channels(void);
extern void test_adc_asynchronous_call(void);
extern void test_adc_sample_with_interval(void);
extern void test_adc_repeated_samplings(void);
extern void test_adc_invalid_request(void);

#include <zephyr.h>
#include <ztest.h>
extern struct device *get_adc_device(void);

void test_main(void)
{
k_object_access_grant(get_adc_device(), k_current_get());

ztest_test_suite(adc_basic_test,
ztest_unit_test(test_adc_sample_one_channel),
ztest_unit_test(test_adc_sample_two_channels),
ztest_user_unit_test(test_adc_sample_one_channel),
ztest_user_unit_test(test_adc_sample_two_channels),
ztest_unit_test(test_adc_asynchronous_call),
ztest_unit_test(test_adc_sample_with_interval),
ztest_unit_test(test_adc_repeated_samplings),
ztest_unit_test(test_adc_invalid_request));
ztest_user_unit_test(test_adc_invalid_request));
ztest_run_test_suite(adc_basic_test);
}
10 changes: 7 additions & 3 deletions tests/drivers/adc/adc_api/src/test_adc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@
#endif

#define BUFFER_SIZE 6
static s16_t m_sample_buffer[BUFFER_SIZE];
static ZTEST_BMEM s16_t m_sample_buffer[BUFFER_SIZE];

static const struct adc_channel_cfg m_1st_channel_cfg = {
.gain = ADC_GAIN,
Expand All @@ -149,6 +149,11 @@ static const struct adc_channel_cfg m_2nd_channel_cfg = {
};
#endif /* defined(ADC_2ND_CHANNEL_ID) */

struct device *get_adc_device(void)
{
return device_get_binding(ADC_DEVICE_NAME);
}

static struct device *init_adc(void)
{
int ret;
Expand Down Expand Up @@ -288,7 +293,6 @@ static int test_task_asynchronous_call(void)
K_POLL_EVENT_INITIALIZER(K_POLL_TYPE_SIGNAL,
K_POLL_MODE_NOTIFY_ONLY,
&async_sig);

struct device *adc_dev = init_adc();

if (!adc_dev) {
Expand Down Expand Up @@ -472,7 +476,7 @@ static int test_task_invalid_request(void)
ret = adc_read(adc_dev, &sequence);
zassert_not_equal(ret, 0, "adc_read() unexpectedly succeeded");

#if defined(CONFIG_ADC_ASYNC)
#if defined(CONFIG_ADC_ASYNC) && !defined(CONFIG_USERSPACE)
ret = adc_read_async(adc_dev, &sequence, &async_sig);
zassert_not_equal(ret, 0, "adc_read_async() unexpectedly succeeded");
#endif
Expand Down