| 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
| ||
|
Date: Thu, 8 Oct 2015 13:59:11 -0400
From: Josh Boyer <jwboyer@...oraproject.org>
To: "Luis R. Rodriguez" <mcgrof@...not-panic.com>
Cc: Greg KH <gregkh@...uxfoundation.org>,
Ming Lei <ming.lei@...onical.com>,
Jonathan Corbet <corbet@....net>,
"Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>,
linux-doc@...r.kernel.org, David Woodhouse <dwmw2@...radead.org>,
David Howells <dhowells@...hat.com>,
Seth Forshee <seth.forshee@...onical.com>,
Rusty Russell <rusty@...tcorp.com.au>,
Michal Marek <mmarek@...e.cz>,
Matthew Garrett <mjg59@...f.ucam.org>, kyle@...nel.org,
linux-security-module <linux-security-module@...r.kernel.org>,
keyrings@...ux-nfs.org, "Luis R. Rodriguez" <mcgrof@...e.com>,
Tom Gundersen <teg@...m.no>
Subject: Re: [PATCH v2 5/5] firmware: add an extensible system data helpers
On Thu, Oct 1, 2015 at 1:44 PM, Luis R. Rodriguez
<mcgrof@...not-panic.com> wrote:
> diff --git a/include/linux/sysdata.h b/include/linux/sysdata.h
> new file mode 100644
> index 000000000000..a69cf5ef082c
> --- /dev/null
> +++ b/include/linux/sysdata.h
> @@ -0,0 +1,208 @@
> +#ifndef _LINUX_SYSDATA_H
> +#define _LINUX_SYSDATA_H
> +
> +#include <linux/types.h>
> +#include <linux/compiler.h>
> +#include <linux/gfp.h>
> +
> +/*
> + * System Data internals
> + *
> + * Copyright (C) 2015 Luis R. Rodriguez <mcgrof@...not-panic.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public Licence
> + * as published by the Free Software Foundation; either version
> + * 2 of the Licence, or (at your option) any later version.
> + */
> +
> +struct sysdata_file {
> + size_t size;
> + const u8 *data;
> +
> + /* sysdata loader private fields */
> + void *priv;
> +};
> +
> +/**
> + * enum sync_data_mode - system data mode of operation
> + *
> + * SYNCDATA_SYNC: your call to request system data is synchronous. We will
> + * look for the system data file you have requested immediatley.
> + * SYNCDATA_ASYNC: your call to request system data is asynchronous. We will
> + * schedule the search for your system data file to be run at a later
> + * time.
> + */
> +enum sync_data_mode {
> + SYNCDATA_SYNC,
> + SYNCDATA_ASYNC,
> +};
> +
> +/* one per sync_data_mode */
> +union sysdata_file_cbs {
> + struct {
> + int __must_check (*found_cb)(void *, const struct sysdata_file *);
> + void *found_context;
> +
> + int __must_check (*opt_fail_cb)(void *);
> + void *opt_fail_context;
> + } sync;
> + struct {
> + void (*found_cb)(const struct sysdata_file *, void *);
> + void *found_context;
> +
> + void (*opt_fail_cb)(void *);
> + void *opt_fail_context;
> + } async;
> +};
> +
> +struct sysdata_file_sync_reqs {
> + enum sync_data_mode mode;
> + struct module *module;
> + gfp_t gfp;
> +};
> +
> +/**
> + * struct sysdata_file_desc - system data file descriptor
> + * @optional: if true it is not a hard requirement by the caller that this
> + * file be present. An error will not be recorded if the file is not
> + * found.
> + * @keep: if set the caller wants to claim ownership over the system data
> + * through one of its callbacks, it must later free it with
> + * release_sysdata_file(). By default this is set to false and the kernel
> + * will release the system data file for you after callback processing
> + * has completed.
> + * @sync_reqs: synchronization requirements, this will be taken care for you
> + * by default if you are usingy sdata_file_request(), otherwise you
> + * should provide your own requirements.
> + *
> + * This structure is set the by the driver and passed to the system data
> + * file helpers sysdata_file_request() or sysdata_file_request_async().
> + * It is intended to carry all requirements and specifications required
> + * to complete the task to get the requested system date file to the caller.
> + * If you wish to extend functionality of system data file requests you
> + * should extend this data structure and make use of the extensions on
> + * the callers to avoid unnecessary collateral evolutions.
> + *
> + * You are allowed to provide a callback to handle if a system data file was
> + * found or not. You do not need to provide a callback. You may also set
> + * an optional flag which would enable you to declare that the system data
> + * file is optional and that if it is not found an alternative callback be
> + * run for you.
> + *
> + * Refer to sysdata_file_request() and sysdata_file_request_async() for more
> + * details.
> + */
> +struct sysdata_file_desc {
> + bool optional;
> + bool keep;
> + struct sysdata_file_sync_reqs sync_reqs;
> + union sysdata_file_cbs cbs;
> +};
> +
> +/*
> + * We keep these template definitions to a minimum for the most
> + * popular requests.
> + */
> +
> +/* Typical sync data case */
> +#define SYSDATA_SYNC_FOUND(__found_cb, __context) \
> + .cbs.sync.found_cb = __found_cb, \
> + .cbs.sync.found_context = __context
> +
> +/* If you have one fallback routine */
> +#define SYSDATA_SYNC_OPT_CB(__found_cb, __context) \
> + .cbs.sync.opt_fail_cb = __found_cb, \
> + .cbs.sync.opt_fail_context = __context
> +
> +/*
> + * Used to define the default asynchronization requirements for
> + * sysdata_file_request_async(). Drivers can override.
> + */
> +#define SYSDATA_DEFAULT_ASYNC(__found_cb, __context) \
> + .sync_reqs = { \
> + .mode = SYNCDATA_ASYNC, \
> + .module = THIS_MODULE, \
> + .gfp = GFP_KERNEL, \
> + }, \
> + .cbs.async = { \
> + .found_cb = __found_cb, \
> + .found_context = __context, \
> + }
> +
> +#define desc_sync_found_cb(desc) (desc->cbs.sync.found_cb)
> +#define desc_sync_found_context(desc) (desc->cbs.sync.found_context)
> +static inline int desc_sync_found_call_cb(const struct sysdata_file_desc *desc,
> + const struct sysdata_file *sysdata)
> +{
> + BUG_ON(desc->sync_reqs.mode != SYNCDATA_SYNC);
ngh... Why do these inline functions all have BUG_ONs in them? If it
is to catch a programming error, why can't you just return EINVAL like
you do in the async function case? (Even that WARN_ON seems
excessive).
Basically you're bringing a user's box down unnecessarily with BUG_ON here.
I need to look at this all more closely, but this caught my eye right away.
josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists