Skip to content

Commit

Permalink
staging: vchiq: fix __user annotations
Browse files Browse the repository at this point in the history 
My earlier patches caused some new sparse warnings, but it turns out
that a number of those are actual bugs, or at least suspicous code.

Adding __user annotations to the data structures that are defined in
uapi headers helps avoid the new warnings, but that causes a different
set of warnings to show up, as some of these structures are used both
inside of the kernel and at the user interface but storing pointers to
different things there.

Duplicating the vchiq_service_params and vchiq_completion_data structures
in turn takes care of most of those, and then it turns out that there
is a 'data' pointer that can be any of a __user address, a dmd_addr_t
and a kernel pointer in vmalloc space at times.

I'm trying to annotate these as best I can without changing behavior,
but there still seems to be a serious bug when user space passes
a valid vmalloc space address instead of a user pointer. Adding
comments in the code there, and leaving the warnings in place that
seem to correspond to actual bugs.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Link: https://lore.kernel.org/r/20200925114424.2647144-1-arnd@arndb.de
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  • Loading branch information
@arndb @gregkh
arndb authored and gregkh committed Sep 25, 2020
1 parent 69fea2b commit 4184da4
Show file tree
Hide file tree
Showing 8 changed files with 108 additions and 62 deletions.
2 changes: 1 addition & 1 deletion drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ static int
vc_vchi_audio_init(struct vchiq_instance *vchiq_instance,
struct bcm2835_audio_instance *instance)
{
struct vchiq_service_params params = {
struct vchiq_service_params_kernel params = {
.version = VC_AUDIOSERV_VER,
.version_min = VC_AUDIOSERV_MIN_VER,
.fourcc = VCHIQ_MAKE_FOURCC('A', 'U', 'D', 'S'),
Expand Down
11 changes: 9 additions & 2 deletions drivers/staging/vc04_services/include/linux/raspberrypi/vchiq.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,14 @@ struct vchiq_service_base {
void *userdata;
};

struct vchiq_service_params {
struct vchiq_completion_data_kernel {
enum vchiq_reason reason;
struct vchiq_header *header;
void *service_userdata;
void *bulk_userdata;
};

struct vchiq_service_params_kernel {
int fourcc;
enum vchiq_status (*callback)(enum vchiq_reason reason,
struct vchiq_header *header,
Expand All @@ -79,7 +86,7 @@ extern enum vchiq_status vchiq_initialise(struct vchiq_instance **pinstance);
extern enum vchiq_status vchiq_shutdown(struct vchiq_instance *instance);
extern enum vchiq_status vchiq_connect(struct vchiq_instance *instance);
extern enum vchiq_status vchiq_open_service(struct vchiq_instance *instance,
const struct vchiq_service_params *params,
const struct vchiq_service_params_kernel *params,
unsigned int *pservice);
extern enum vchiq_status vchiq_close_service(unsigned int service);
extern enum vchiq_status vchiq_use_service(unsigned int service);
Expand Down
2 changes: 1 addition & 1 deletion drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ vchiq_prepare_bulk_data(struct vchiq_bulk *bulk, void *offset, int size,
if (!pagelistinfo)
return VCHIQ_ERROR;

bulk->data = (void *)(unsigned long)pagelistinfo->dma_addr;
bulk->data = pagelistinfo->dma_addr;

/*
* Store the pagelistinfo address in remote_data,
Expand Down
95 changes: 61 additions & 34 deletions drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ int vchiq_susp_log_level = VCHIQ_LOG_ERROR;

struct user_service {
struct vchiq_service *service;
void *userdata;
void __user *userdata;
struct vchiq_instance *instance;
char is_vchi;
char dequeue_pending;
Expand All @@ -75,7 +75,7 @@ struct bulk_waiter_node {

struct vchiq_instance {
struct vchiq_state *state;
struct vchiq_completion_data completions[MAX_COMPLETIONS];
struct vchiq_completion_data_kernel completions[MAX_COMPLETIONS];
int completion_insert;
int completion_remove;
struct completion insert_event;
Expand Down Expand Up @@ -273,7 +273,7 @@ EXPORT_SYMBOL(vchiq_connect);

static enum vchiq_status vchiq_add_service(
struct vchiq_instance *instance,
const struct vchiq_service_params *params,
const struct vchiq_service_params_kernel *params,
unsigned int *phandle)
{
enum vchiq_status status;
Expand Down Expand Up @@ -311,7 +311,7 @@ static enum vchiq_status vchiq_add_service(

enum vchiq_status vchiq_open_service(
struct vchiq_instance *instance,
const struct vchiq_service_params *params,
const struct vchiq_service_params_kernel *params,
unsigned int *phandle)
{
enum vchiq_status status = VCHIQ_ERROR;
Expand Down Expand Up @@ -359,7 +359,8 @@ vchiq_bulk_transmit(unsigned int handle, const void *data,
switch (mode) {
case VCHIQ_BULK_MODE_NOCALLBACK:
case VCHIQ_BULK_MODE_CALLBACK:
status = vchiq_bulk_transfer(handle, (void *)data, size,
status = vchiq_bulk_transfer(handle,
(void *)data, size,
userdata, mode,
VCHIQ_BULK_TRANSMIT);
break;
Expand Down Expand Up @@ -453,7 +454,8 @@ vchiq_blocking_bulk_transfer(unsigned int handle, void *data,

if (bulk) {
/* This thread has an outstanding bulk transfer. */
if ((bulk->data != data) ||
/* FIXME: why compare a dma address to a pointer? */
if ((bulk->data != (dma_addr_t)(uintptr_t)data) ||
(bulk->size != size)) {
/* This is not a retry of the previous one.
* Cancel the signal when the transfer
Expand Down Expand Up @@ -513,7 +515,7 @@ add_completion(struct vchiq_instance *instance, enum vchiq_reason reason,
struct vchiq_header *header, struct user_service *user_service,
void *bulk_userdata)
{
struct vchiq_completion_data *completion;
struct vchiq_completion_data_kernel *completion;
int insert;

DEBUG_INITIALISE(g_state.local)
Expand Down Expand Up @@ -802,7 +804,7 @@ static int vchiq_ioc_create_service(struct vchiq_instance *instance,
struct user_service *user_service = NULL;
struct vchiq_service *service;
enum vchiq_status status = VCHIQ_SUCCESS;
void *userdata;
struct vchiq_service_params_kernel params;
int srvstate;

user_service = kmalloc(sizeof(*user_service), GFP_KERNEL);
Expand All @@ -820,20 +822,23 @@ static int vchiq_ioc_create_service(struct vchiq_instance *instance,
VCHIQ_SRVSTATE_LISTENING : VCHIQ_SRVSTATE_HIDDEN;
}

userdata = args->params.userdata;
args->params.callback = service_callback;
args->params.userdata = user_service;
service = vchiq_add_service_internal(instance->state, &args->params,
params = (struct vchiq_service_params_kernel) {
.fourcc = args->params.fourcc,
.callback = service_callback,
.userdata = user_service,
.version = args->params.version,
.version_min = args->params.version_min,
};
service = vchiq_add_service_internal(instance->state, &params,
srvstate, instance,
user_service_free);

if (!service) {
kfree(user_service);
return -EEXIST;
}

user_service->service = service;
user_service->userdata = userdata;
user_service->userdata = args->params.userdata;
user_service->instance = instance;
user_service->is_vchi = (args->is_vchi != 0);
user_service->dequeue_pending = 0;
Expand Down Expand Up @@ -945,6 +950,7 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
{
struct vchiq_service *service;
struct bulk_waiter_node *waiter = NULL;
void *userdata;
int status = 0;
int ret;

Expand All @@ -960,7 +966,7 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
goto out;
}

args->userdata = &waiter->bulk_waiter;
userdata = &waiter->bulk_waiter;
} else if (args->mode == VCHIQ_BULK_MODE_WAITING) {
mutex_lock(&instance->bulk_waiter_list_mutex);
list_for_each_entry(waiter, &instance->bulk_waiter_list,
Expand All @@ -981,11 +987,18 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
vchiq_log_info(vchiq_arm_log_level,
"found bulk_waiter %pK for pid %d", waiter,
current->pid);
args->userdata = &waiter->bulk_waiter;
userdata = &waiter->bulk_waiter;
}

/*
* FIXME address space mismatch:
* args->data may be interpreted as a kernel pointer
* in create_pagelist() called from vchiq_bulk_transfer(),
* accessing kernel data instead of user space, based on the
* address.
*/
status = vchiq_bulk_transfer(args->handle, args->data, args->size,
args->userdata, args->mode, dir);
userdata, args->mode, dir);

if (!waiter) {
ret = 0;
Expand Down Expand Up @@ -1027,19 +1040,22 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
return 0;
}

/* read a user pointer value from an array pointers in user space */
static inline int vchiq_get_user_ptr(void __user **buf, void __user *ubuf, int index)
{
compat_uptr_t ptr32;
int ret;

if (in_compat_syscall()) {
compat_uptr_t ptr32;
compat_uptr_t __user *uptr = ubuf;
ret = get_user(ptr32, &uptr[index]);
ret = get_user(ptr32, uptr + index);
*buf = compat_ptr(ptr32);
} else {
void __user *__user *uptr = ubuf;
ret = get_user(buf, &uptr[index]);
uintptr_t ptr, __user *uptr = ubuf;
ret = get_user(ptr, uptr + index);
*buf = (void __user *)ptr;
}

return ret;
}

Expand All @@ -1058,10 +1074,10 @@ static int vchiq_put_completion(struct vchiq_completion_data __user *buf,

if (in_compat_syscall()) {
struct vchiq_completion_data32 tmp = {
.reason = buf->reason,
.header = ptr_to_compat(buf->header),
.service_userdata = ptr_to_compat(buf->service_userdata),
.bulk_userdata = ptr_to_compat(buf->bulk_userdata),
.reason = completion->reason,
.header = ptr_to_compat(completion->header),
.service_userdata = ptr_to_compat(completion->service_userdata),
.bulk_userdata = ptr_to_compat(completion->bulk_userdata),
};
if (copy_to_user(&buf32[index], &tmp, sizeof(tmp)))
return -EFAULT;
Expand Down Expand Up @@ -1115,7 +1131,8 @@ static int vchiq_ioc_await_completion(struct vchiq_instance *instance,
remove = instance->completion_remove;

for (ret = 0; ret < args->count; ret++) {
struct vchiq_completion_data *completion;
struct vchiq_completion_data_kernel *completion;
struct vchiq_completion_data user_completion;
struct vchiq_service *service;
struct user_service *user_service;
struct vchiq_header *header;
Expand All @@ -1134,7 +1151,12 @@ static int vchiq_ioc_await_completion(struct vchiq_instance *instance,

service = completion->service_userdata;
user_service = service->base.userdata;
completion->service_userdata = user_service->userdata;

memset(&user_completion, 0, sizeof(user_completion));
user_completion = (struct vchiq_completion_data) {
.reason = completion->reason,
.service_userdata = user_service->userdata,
};

header = completion->header;
if (header) {
Expand All @@ -1158,7 +1180,7 @@ static int vchiq_ioc_await_completion(struct vchiq_instance *instance,
break;
/* Get the pointer from user space */
msgbufcount--;
if (vchiq_get_user_ptr(&msgbuf, &args->msgbufs,
if (vchiq_get_user_ptr(&msgbuf, args->msgbufs,
msgbufcount)) {
if (ret == 0)
ret = -EFAULT;
Expand All @@ -1178,15 +1200,20 @@ static int vchiq_ioc_await_completion(struct vchiq_instance *instance,

/* The completion must point to the
** msgbuf. */
completion->header =
(struct vchiq_header __force *)msgbuf;
user_completion.header = msgbuf;
}

if ((completion->reason == VCHIQ_SERVICE_CLOSED) &&
!instance->use_close_delivered)
unlock_service(service);

if (vchiq_put_completion(args->buf, completion, ret)) {
/*
* FIXME: address space mismatch, does bulk_userdata
* actually point to user or kernel memory?
*/
user_completion.bulk_userdata = completion->bulk_userdata;

if (vchiq_put_completion(args->buf, &user_completion, ret)) {
if (ret == 0)
ret = -EFAULT;
break;
Expand Down Expand Up @@ -1713,7 +1740,7 @@ struct vchiq_await_completion32 {
static long
vchiq_compat_ioctl_await_completion(struct file *file,
unsigned int cmd,
struct vchiq_await_completion32 *argp)
struct vchiq_await_completion32 __user *argp)
{
struct vchiq_await_completion args;
struct vchiq_await_completion32 args32;
Expand Down Expand Up @@ -1926,7 +1953,7 @@ static int vchiq_release(struct inode *inode, struct file *file)
/* Release any closed services */
while (instance->completion_remove !=
instance->completion_insert) {
struct vchiq_completion_data *completion;
struct vchiq_completion_data_kernel *completion;
struct vchiq_service *service;

completion = &instance->completions[
Expand Down Expand Up @@ -2191,7 +2218,7 @@ vchiq_keepalive_thread_func(void *v)
struct vchiq_instance *instance;
unsigned int ka_handle;

struct vchiq_service_params params = {
struct vchiq_service_params_kernel params = {
.fourcc = VCHIQ_MAKE_FOURCC('K', 'E', 'E', 'P'),
.callback = vchiq_keepalive_vchiq_callback,
.version = KEEPALIVE_VER,
Expand Down
19 changes: 10 additions & 9 deletions drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1392,7 +1392,7 @@ abort_outstanding_bulks(struct vchiq_service *service,
bulk->remote_size);
} else {
/* fabricate a matching dummy bulk */
bulk->data = NULL;
bulk->data = 0;
bulk->size = 0;
bulk->actual = VCHIQ_BULK_ACTUAL_ABORTED;
bulk->dir = is_tx ? VCHIQ_BULK_TRANSMIT :
Expand Down Expand Up @@ -1764,10 +1764,10 @@ parse_rx_slots(struct vchiq_state *state)
queue->remote_insert++;

vchiq_log_info(vchiq_core_log_level,
"%d: prs %s@%pK (%d->%d) %x@%pK",
"%d: prs %s@%pK (%d->%d) %x@%pad",
state->id, msg_type_str(type),
header, remoteport, localport,
bulk->actual, bulk->data);
bulk->actual, &bulk->data);

vchiq_log_trace(vchiq_core_log_level,
"%d: prs:%d %cx li=%x ri=%x p=%x",
Expand Down Expand Up @@ -2316,7 +2316,7 @@ struct vchiq_header *vchiq_msg_hold(unsigned int handle)
}
EXPORT_SYMBOL(vchiq_msg_hold);

static int vchiq_validate_params(const struct vchiq_service_params *params)
static int vchiq_validate_params(const struct vchiq_service_params_kernel *params)
{
if (!params->callback || !params->fourcc) {
vchiq_loud_error("Can't add service, invalid params\n");
Expand All @@ -2329,7 +2329,7 @@ static int vchiq_validate_params(const struct vchiq_service_params *params)
/* Called from application thread when a client or server service is created. */
struct vchiq_service *
vchiq_add_service_internal(struct vchiq_state *state,
const struct vchiq_service_params *params,
const struct vchiq_service_params_kernel *params,
int srvstate, struct vchiq_instance *instance,
vchiq_userdata_term userdata_term)
{
Expand Down Expand Up @@ -3015,7 +3015,8 @@ vchiq_remove_service(unsigned int handle)
* structure.
*/
enum vchiq_status vchiq_bulk_transfer(unsigned int handle,
void *offset, int size, void *userdata,
void *offset, int size,
void *userdata,
enum vchiq_bulk_mode mode,
enum vchiq_bulk_dir dir)
{
Expand Down Expand Up @@ -3093,9 +3094,9 @@ enum vchiq_status vchiq_bulk_transfer(unsigned int handle,
wmb();

vchiq_log_info(vchiq_core_log_level,
"%d: bt (%d->%d) %cx %x@%pK %pK",
"%d: bt (%d->%d) %cx %x@%pad %pK",
state->id, service->localport, service->remoteport, dir_char,
size, bulk->data, userdata);
size, &bulk->data, userdata);

/* The slot mutex must be held when the service is being closed, so
claim it here to ensure that isn't happening */
Expand All @@ -3107,7 +3108,7 @@ enum vchiq_status vchiq_bulk_transfer(unsigned int handle,
if (service->srvstate != VCHIQ_SRVSTATE_OPEN)
goto unlock_both_error_exit;

payload[0] = (int)(long)bulk->data;
payload[0] = lower_32_bits(bulk->data);
payload[1] = bulk->size;
status = queue_message(state,
NULL,
Expand Down

0 comments on commit 4184da4

Please sign in to comment.