| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ft2lkhphs25xmvmpbf5ssuafdnmkdnq5j7uqb6jjy6ewnwzn2l@6uoad3ii3h4m>
Date: Fri, 9 Aug 2024 01:46:44 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Mary Strodl <mstrodl@....rit.edu>
Cc: linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
urezki@...il.com, hch@...radead.org, linux-mm@...ck.org, lee@...nel.org,
linux-i2c@...r.kernel.org, s.hauer@...gutronix.de, christian.gmeiner@...il.com
Subject: Re: [PATCH v3 1/2] x86: Add basic support for the Congatec CGEB BIOS
interface
Hi Mary,
...
> --- /dev/null
> +++ b/drivers/mfd/congatec-cgeb.c
> @@ -0,0 +1,1138 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * CGEB driver
> + *
> + * Copyright (c) 2024 Mary Strodl
> + *
> + * Based on code from Congatec AG and Sascha Hauer
> + *
> + * CGEB is a BIOS interface found on congatech modules. It consists of
> + * code found in the BIOS memory map which is called in a ioctl like
> + * fashion. This file contains the basic driver for this interface
> + * which provides access to the GCEB interface and registers the child
> + * devices like I2C busses and watchdogs.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
If you use the SPDX identifier you don't need to mention the GPL
parts here.
> + */
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/module.h>
> +#include <linux/sched.h>
> +#include <linux/io.h>
> +#include <linux/string.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +#include <linux/mm.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/connector.h>
> +#include <linux/mfd/congatec-cgeb.h>
> +#include <linux/completion.h>
> +
> +#include <generated/autoconf.h>
the include files are normally alphabetically sorted.
> +#pragma pack(push, 4)
> +
> +struct cgeb_low_desc {
> + char magic[8]; /* descriptor magic string */
> + u16 size; /* size of this descriptor */
> + u16 reserved;
> + char bios_name[8]; /* BIOS name and revision "ppppRvvv" */
> + u32 hi_desc_phys_addr; /* phys addr of the high descriptor, can be 0 */
> +};
> +
> +/* CGEB High Descriptor located in 0xfff00000-0xffffffff */
> +#ifdef CONFIG_X86_64
> +#define CGEB_HD_MAGIC "$CGEBQD$"
> +#else
> +#define CGEB_HD_MAGIC "$CGEBHD$"
> +#endif
> +
> +struct cgeb_high_desc {
> + char magic[8]; /* descriptor magic string */
> + u16 size; /* size of this descriptor */
> + u16 reserved;
> + u32 data_size; /* CGEB data area size */
> + u32 code_size; /* CGEB code area size */
> + u32 entry_rel; /* CGEB entry point relative to start */
> +};
> +
> +struct cgeb_far_ptr {
> + void __user *off;
> + u16 seg;
> + u16 pad;
> +};
> +
> +struct cgeb_fps {
> + u32 size; /* size of the parameter structure */
> + u32 fct; /* function number */
> + struct cgeb_far_ptr data; /* CGEB data area */
> + void __user *cont; /* private continuation pointer */
> + void __user *subfps; /* private sub function parameter
> + * structure pointer
> + */
> + void __user *subfct; /* sub function pointer */
> + u32 status; /* result codes of the function */
> + u32 unit; /* unit number or type */
> + u32 pars[4]; /* input parameters */
> + u32 rets[2]; /* return parameters */
> + void __user *iptr; /* input pointer */
> + void __user *optr; /* output pointer */
> +};
...
> +struct cgeb_map_mem {
> + void *phys; /* physical address */
> + u32 size; /* size in bytes */
> + struct cgeb_far_ptr virt;
> +};
> +
> +struct cgeb_map_mem_list {
> + u32 count; /* number of memory map entries */
> + struct cgeb_map_mem entries[];
> +};
> +
> +struct cgeb_boardinfo {
> + u32 size;
> + u32 flags;
> + u32 classes;
> + u32 primary_class;
> + char board[CGOS_BOARD_MAX_SIZE_ID_STRING];
> + /* optional */
> + char vendor[CGOS_BOARD_MAX_SIZE_ID_STRING];
> +};
> +
> +struct cgeb_i2c_info {
> + u32 size;
> + u32 type;
> + u32 frequency;
> + u32 maxFrequency;
> +};
...
> +struct cgeb_board_data {
> + void __user *data;
> + void __user *code;
> + size_t code_size;
> + u16 ds;
> + struct cgeb_map_mem_list *map_mem;
> + void __user *map_mem_user;
> + struct platform_device **devices;
> + int num_devices;
> +
> + #ifdef CONFIG_X86_64
> + void (*entry)(void*, struct cgeb_fps *, struct cgeb_fps *, void*);
> + #else
> + /*
> + * entry points to a bimodal C style function that expects a far pointer
> + * to a fps. If cs is 0 then it does a near return, otherwise a far
> + * return. If we ever need a far return then we must not pass cs at all.
> + * parameters are removed by the caller.
> + */
> + void __attribute__((regparm(0)))(*entry)(unsigned short,
> + struct cgeb_fps *, unsigned short);
> + #endif
> +};
> +
> +struct cgeb_call_user {
> + void *optr;
> + size_t size;
> + void *callback_data;
> + int (*callback)(void __user *optr, void *koptr, void *data);
> +};
> +
> +enum cgeb_msg_type {
> + CGEB_MSG_ACK = 0,
> + CGEB_MSG_ERROR,
> + CGEB_MSG_FPS,
> + CGEB_MSG_MAPPED,
> + CGEB_MSG_MAP,
> + CGEB_MSG_CODE,
> + CGEB_MSG_ALLOC,
> + CGEB_MSG_ALLOC_CODE,
> + CGEB_MSG_FREE,
> + CGEB_MSG_MUNMAP,
> + CGEB_MSG_CALL,
> + CGEB_MSG_PING,
> +};
> +
> +struct cgeb_msg {
> + enum cgeb_msg_type type;
> + union {
> + struct cgeb_msg_mapped {
> + void __user *virt;
> + } mapped;
> + struct cgeb_msg_fps {
> + size_t optr_size;
> + void __user *optr;
> + struct cgeb_fps fps;
> + } fps;
> + struct cgeb_msg_code {
> + size_t length;
> + uint32_t entry_rel;
> + void __user *data;
> + } code;
> + struct cgeb_msg_map {
> + uint32_t phys;
> + size_t size;
> + } map;
> + };
> +};
> +
> +static char cgeb_helper_path[PATH_MAX] = "/sbin/cgeb-helper";
> +
> +static struct cb_id cgeb_cn_id = {
> + .idx = CN_IDX_CGEB,
> + .val = CN_VAL_CGEB
> +};
> +
> +enum cgeb_request_state {
> + CGEB_REQ_IDLE = 0,
> + CGEB_REQ_ACTIVE,
> + CGEB_REQ_DONE,
> +};
> +
> +static DEFINE_MUTEX(cgeb_lock);
> +struct cgeb_request {
> + struct completion done;
> + struct cgeb_msg *out;
> + enum cgeb_request_state busy;
> + int ack;
> + int (*callback)(struct cgeb_msg *msg, void *user);
> + void *user;
> +};
> +
> +#define CGEB_REQUEST_MAX 16
> +static struct cgeb_request cgeb_requests[CGEB_REQUEST_MAX];
> +
> +struct cgeb_after_alloc_data {
> + void *kernel;
> + void __user **user;
> + size_t length;
> +};
> +
> +struct cgeb_map_data {
> + void __user *user_list;
> + struct cgeb_board_data *board;
> +};
I'm counting too many global variables here and too many global
structure definitions. It might get a bit hard to follow.
I'd suggest to simplify this part as much as possible and keep
everything in as less structures as possible (ideally just one).
Please make local as many variables as you can.
...
> +static int cgeb_request(struct cgeb_msg msg, struct cgeb_msg *out,
> + int (*callback)(struct cgeb_msg*, void*), void *user)
> +{
...
> + if (req->busy) {
> + mutex_unlock(&cgeb_lock);
> + err = -EBUSY;
> + goto out;
just return -EBUSY
> + }
> + wrapper->seq = seq;
> + req->busy = CGEB_REQ_ACTIVE;
> + req->ack = wrapper->ack;
> + req->out = out;
> + req->callback = callback;
> + req->user = user;
> +
> + err = cn_netlink_send(wrapper, 0, 0, GFP_KERNEL);
> + if (err == -ESRCH) {
> + err = cgeb_helper_start();
> + if (err) {
> + pr_err("failed to execute %s\n", cgeb_helper_path);
> + pr_err("make sure the cgeb helper is executable\n");
> + } else {
> + do {
> + err = cn_netlink_send(wrapper, 0, 0,
> + GFP_KERNEL);
> + if (err == -ENOBUFS)
> + err = 0;
> + if (err == -ESRCH)
> + msleep(30);
> + } while (err == -ESRCH && ++retries < 5);
when you use constants like "5", you need to give it an
explanation, either as a define (the name speaks by itself) or a
comment. We don't want people asking "why 5?" :-)
> + }
> + } else if (err == -ENOBUFS)
> + err = 0;
based on the coding style this should have been:
if (err == -ESRCH) {
...
} else if (err == -ENOBUFS) {
err = 0;
}
You either use brackets for every if/else/else fi or for no one.
> + kfree(wrapper);
> +
> + if (++seq >= CGEB_REQUEST_MAX)
> + seq = 0;
> +
> + mutex_unlock(&cgeb_lock);
> +
> + if (err)
> + goto out;
just return err
> +
> + /* Wait for a response to the request */
> + err = wait_for_completion_interruptible_timeout(
> + &req->done, msecs_to_jiffies(20000));
> + if (err == 0) {
> + pr_err("CGEB: Timed out running request of type %d!\n",
> + msg.type);
> + err = -ETIMEDOUT;
just "return -ETIMEDOUT;" and you can cleanup the code below
> + } else if (err > 0)
> + err = 0;
brackets!
> + if (err)
> + goto out;
> +
> + mutex_lock(&cgeb_lock);
> +
> + if (req->busy != CGEB_REQ_DONE) {
> + pr_err("CGEB: BUG: Request is in a bad state?\n");
> + err = -EINVAL;
> + }
> +
> + req->busy = CGEB_REQ_IDLE;
> + mutex_unlock(&cgeb_lock);
> +out:
> + return err;
I don't see any need for the out: label
> +}
> +
> +static void cgeb_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
> +{
> + struct cgeb_request *req;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return;
> +
> + if (msg->seq >= CGEB_REQUEST_MAX) {
> + pr_err("CGEB: Impossible sequence number: %d! Ignoring\n",
> + msg->seq);
> + return;
> + }
> +
> + mutex_lock(&cgeb_lock);
> + req = &cgeb_requests[msg->seq];
> +
> + if (!req->busy || req->ack != msg->ack) {
> + pr_err("CGEB: Bad response to request %d! Ignoring\n",
> + msg->seq);
> + mutex_unlock(&cgeb_lock);
> + return;
> + }
> +
> + if (msg->len != sizeof(*req->out)) {
> + pr_err("CGEB: Bad response size for request %d!\n", msg->seq);
> + mutex_unlock(&cgeb_lock);
> + return;
here it can be useful to have a goto statement:
goto out;
...
out:
mutex_unlock(...);
return;
Andi
Powered by blists - more mailing lists