Re: [PATCH v3 3/5] firmware: fold successful fw read early
On Wed, Dec 23, 2015 at 4:34 PM, Luis R. Rodriguez
wrote:
> From: David Howells
>
> We'll be folding in some more checks on fw_read_file_contents(),
> this will make the success case easier to follow.
>
> Signed-off-by: David Howells
> Signed-off-by: Luis R. Rodriguez
Reviewed-by: Josh Boyer
> ---
> drivers/base/firmware_class.c | 16 +++-
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index d8148aa89b01..e10a5349aa61 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -361,20 +361,18 @@ static int fw_get_filesystem_firmware(struct device
> *device,
> continue;
> rc = fw_read_file_contents(file, buf);
> fput(file);
> - if (rc)
> + if (rc == 0) {
> + dev_dbg(device, "system data: direct-loading firmware
> %s\n",
> + buf->fw_id);
> + fw_finish_direct_load(device, buf);
> + goto out;
> + } else
> dev_warn(device, "system data, attempted to load %s,
> but failed with error %d\n",
> path, rc);
> - else
> - break;
> }
> +out:
> __putname(path);
>
> - if (!rc) {
> - dev_dbg(device, "system data: direct-loading firmware %s\n",
> - buf->fw_id);
> - fw_finish_direct_load(device, buf);
> - }
> -
> return rc;
> }
>
> --
> 2.6.2
>
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/5] firmware: move completing fw into a helper
On Wed, Dec 23, 2015 at 4:34 PM, Luis R. Rodriguez
wrote:
> From: "Luis R. Rodriguez"
>
> This will be re-used later through a new extensible interface.
>
> Signed-off-by: Luis R. Rodriguez
Reviewed-by: Josh Boyer
josh
> ---
> drivers/base/firmware_class.c | 14 ++
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 6f5fcda71a60..d8148aa89b01 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -322,6 +322,15 @@ fail:
> return rc;
> }
>
> +static void fw_finish_direct_load(struct device *device,
> + struct firmware_buf *buf)
> +{
> + mutex_lock(&fw_lock);
> + set_bit(FW_STATUS_DONE, &buf->status);
> + complete_all(&buf->completion);
> + mutex_unlock(&fw_lock);
> +}
> +
> static int fw_get_filesystem_firmware(struct device *device,
>struct firmware_buf *buf)
> {
> @@ -363,10 +372,7 @@ static int fw_get_filesystem_firmware(struct device
> *device,
> if (!rc) {
> dev_dbg(device, "system data: direct-loading firmware %s\n",
> buf->fw_id);
> - mutex_lock(&fw_lock);
> - set_bit(FW_STATUS_DONE, &buf->status);
> - complete_all(&buf->completion);
> - mutex_unlock(&fw_lock);
> + fw_finish_direct_load(device, buf);
> }
>
> return rc;
> --
> 2.6.2
>
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] X.509: Fix determination of self-signedness
On Thu, Dec 17, 2015 at 7:03 PM, David Howells wrote:
> Fix determination of whether an X.509 certificate is self-signed or not.
>
> It is currently assumed that a cert is self-signed if has no
> authorityKeyIdentifier or the authorityKeyIdentifier matches the
> subjectKeyIdentifier. However, it is possible to encounter a certificate
> that has neither AKID not SKID but is not self-signed.
>
> This symptoms of this show up as an attempt to load a certificate failing
> with -ERANGE or -EBADMSG, produced from the RSA module when the result of
> calculating "m = s^e mod n" is checked.
>
> To fix this, don't check to see if a certificate is self-signed if the
> Issuer and Subject names differ.
>
> Signed-off-by: David Howells
> cc: David Woodhouse
Should this also be Cc'd to stable?
josh
> ---
>
> crypto/asymmetric_keys/x509_public_key.c | 11 ---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/crypto/asymmetric_keys/x509_public_key.c
> b/crypto/asymmetric_keys/x509_public_key.c
> index 2a44b3752471..6236e7996f19 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -313,9 +313,14 @@ static int x509_key_preparse(struct
> key_preparsed_payload *prep)
> cert->pub->id_type = PKEY_ID_X509;
>
> /* Check the signature on the key if it appears to be self-signed */
> - if ((!cert->akid_skid && !cert->akid_id) ||
> - asymmetric_key_id_same(cert->skid, cert->akid_skid) ||
> - asymmetric_key_id_same(cert->id, cert->akid_id)) {
> + if ((!cert->akid_skid && !cert->akid_id)) {
> + if (cert->raw_issuer_size == cert->raw_subject_size &&
> + memcmp(cert->raw_issuer, cert->raw_subject,
> + cert->raw_subject_size) == 0)
> + goto self_signed;
> + } else if (asymmetric_key_id_same(cert->skid, cert->akid_skid) ||
> + asymmetric_key_id_same(cert->id, cert->akid_id)) {
> +self_signed:
> ret = x509_check_signature(cert->pub, cert); /* self-signed */
> if (ret < 0)
> goto error_free_cert;
>
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] X.509: Fix the time validation [ver #3]
On Fri, Dec 11, 2015 at 6:13 AM, David Howells wrote: > Greg Kroah-Hartman wrote: > >> David, any reason you didn't put a cc: stable in the commit for it to be >> picked up in the stable releases? > > I did cc it to stable. You had the stable list in the CC field when you sent the patch, but the CC: sta...@vger.kernel.org tag is missing in the commit body. Greg's scripts key off the later. Just cc'ing the list when you send the email does nothing. FWIW, I asked James if it should have been CC'd to stable when he sent the pull request to Linus and I never got a reply. josh -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] security: KEYS: Fix handling of stored error in a negatively instantiated user key
On Wed, Nov 25, 2015 at 6:41 PM, James Morris wrote: > Please pull this fix for the keys subsystem, for 4.4, from David Howells. > > Note: this oops is triggerable by non-privileged users. > > The following changes since commit 6ffeba9607343f15303a399bc402a538800d89d9: > > Merge tag 'dm-4.4-fixes' of > git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm > (2015-11-24 12:53:11 -0800) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git > for-linus > > David Howells (1): > KEYS: Fix handling of stored error in a negatively instantiated user key > > security/keys/encrypted-keys/encrypted.c |2 ++ > security/keys/trusted.c |5 - > security/keys/user_defined.c |5 - > 3 files changed, 10 insertions(+), 2 deletions(-) > > --- > commit 096fe9eaea40a17e125569f9e657e34cdb6d73bd > Author: David Howells > Date: Tue Nov 24 21:36:31 2015 + > > KEYS: Fix handling of stored error in a negatively instantiated user key > > If a user key gets negatively instantiated, an error code is cached in the > payload area. A negatively instantiated key may be then be positively > instantiated by updating it with valid data. However, the ->update key > type method must be aware that the error code may be there. > > The following may be used to trigger the bug in the user key type: > > keyctl request2 user user "" @u > keyctl add user user "a" @u > > which manifests itself as: > > BUG: unable to handle kernel paging request at ff8a > IP: [] __call_rcu.constprop.76+0x1f/0x280 > kernel/rcu/tree.c:3046 > PGD 7cc30067 PUD 0 > Oops: 0002 [#1] SMP > Modules linked in: > CPU: 3 PID: 2644 Comm: a.out Not tainted 4.3.0+ #49 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs > 01/01/2011 > task: 88003ddea700 ti: 88003dd88000 task.ti: 88003dd88000 > RIP: 0010:[] [] > __call_rcu.constprop.76+0x1f/0x280 > [] __call_rcu.constprop.76+0x1f/0x280 > kernel/rcu/tree.c:3046 > RSP: 0018:88003dd8bdb0 EFLAGS: 00010246 > RAX: ff82 RBX: RCX: 0001 > RDX: 81e3fe40 RSI: RDI: ff82 > RBP: 88003dd8bde0 R08: 88007d2d2da0 R09: > R10: R11: 88003e8073c0 R12: ff82 > R13: 88003dd8be68 R14: 88007d027600 R15: 88003ddea700 > FS: 00b92880(0063) GS:88007fd0() > knlGS: > CS: 0010 DS: ES: CR0: 8005003b > CR2: ff8a CR3: 7cc5f000 CR4: 06e0 > Stack: > 88003dd8bdf0 81160a8a ff82 > 88003dd8be68 88007d027600 88003dd8bdf0 810a39e5 > 88003dd8be20 812a31ab 88007d027600 88007d027620 > Call Trace: > [] kfree_call_rcu+0x15/0x20 kernel/rcu/tree.c:3136 > [] user_update+0x8b/0xb0 > security/keys/user_defined.c:129 > [< inline >] __key_update security/keys/key.c:730 > [] key_create_or_update+0x291/0x440 > security/keys/key.c:908 > [< inline >] SYSC_add_key security/keys/keyctl.c:125 > [] SyS_add_key+0x101/0x1e0 > security/keys/keyctl.c:60 > [] entry_SYSCALL_64_fastpath+0x12/0x6a > arch/x86/entry/entry_64.S:185 > > Note the error code (-ENOKEY) in EDX. > > A similar bug can be tripped by: > > keyctl request2 trusted user "" @u > keyctl add trusted user "a" @u > > This should also affect encrypted keys - but that has to be correctly > parameterised or it will fail with EINVAL before getting to the bit that > will crashes. > > Reported-by: Dmitry Vyukov > Signed-off-by: David Howells > Acked-by: Mimi Zohar > Signed-off-by: James Morris This should probably be sent to stable, yes? josh -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/10] KEYS: Change how keys are determined to be trusted
On Wed, Oct 21, 2015 at 2:11 PM, Mimi Zohar wrote: > On Wed, 2015-10-21 at 13:21 -0400, Josh Boyer wrote: >> On Wed, Oct 21, 2015 at 1:02 PM, Mimi Zohar wrote: >> > On Wed, 2015-10-21 at 16:13 +0100, David Howells wrote: >> >> Here's a set of patches that changes how keys are determined to be trusted >> >> - currently, that's a case of whether a key has KEY_FLAG_TRUSTED set upon >> >> it. A keyring can then have a flag set (KEY_FLAG_TRUSTED ONLY) that >> >> indicates that only keys with this flag set may be added to that keyring. >> >> >> >> Further, any time an X.509 certificate is instantiated without this flag >> >> set, the certificate is judged against the contents of the system trusted >> >> keyring to determine whether KEY_FLAG_TRUSTED should be set upon it. >> >> >> >> With these patches, KEY_FLAG_TRUSTED is removed. The kernel may add >> >> implicitly trusted keys to a trusted-only keyring by asserting >> >> KEY_ALLOC_TRUSTED when the key is created, >> > >> > Ok, but only the x509 certificates built into the kernel image should be >> > automatically trusted and can be added to a trusted keyring, because the >> > kernel itself was signed (and verified). These certificates extend the >> > (UEFI) certificate chain of trust that is rooted in hardware to the OS. >> >> That doesn't sound accurate to me. The cert built into the kernel >> image doesn't extend the UEFI certificates. In most cases, it is a >> ephemeral cert that is automatically generated at kernel build time >> and then discarded. It is not chained to or derived from any of the >> UEFI certs stored in the db (or mok) variables. The built-in cert is >> used for module loading verification. I agree that it should be >> trusted, but not really for the reason you list. Perhaps you meant >> the key that the PE image of the kernel is signed with? If so, the >> kernel doesn't load that. Only shim (and grub2 via shim) read that >> key. > > This is similar to the concept of the MoK DB. Keys added to the MoK > aren't signed by a UEFI key, yet they extend the UEFI secure boot > certificate chain of trust. Similarly, the certificates built into the Right, because UEFI is verifying shim, which verifies grub2, which verifies the kernel. I get that. However, it's irrelevant. > kernel image don't need to be signed by a UEFI/MoK key for it to extend > the certificate chain of trust. The certificates built _into_ the kernel need to be trusted in all cases. It is how module signing is done. So a user not using Secure Boot, or even not using UEFI, still needs those embedded certs trusted so that they can load modules. It has nothing to do with UEFI or some single-root-of-trust. At any rate, I believe we are both saying the embedded cert needs to be trusted so there's little point in debating further. I just wanted to point out that this need has nothing to do with UEFI. josh -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/10] KEYS: Change how keys are determined to be trusted
On Wed, Oct 21, 2015 at 1:02 PM, Mimi Zohar wrote: > On Wed, 2015-10-21 at 16:13 +0100, David Howells wrote: >> Here's a set of patches that changes how keys are determined to be trusted >> - currently, that's a case of whether a key has KEY_FLAG_TRUSTED set upon >> it. A keyring can then have a flag set (KEY_FLAG_TRUSTED ONLY) that >> indicates that only keys with this flag set may be added to that keyring. >> >> Further, any time an X.509 certificate is instantiated without this flag >> set, the certificate is judged against the contents of the system trusted >> keyring to determine whether KEY_FLAG_TRUSTED should be set upon it. >> >> With these patches, KEY_FLAG_TRUSTED is removed. The kernel may add >> implicitly trusted keys to a trusted-only keyring by asserting >> KEY_ALLOC_TRUSTED when the key is created, > > Ok, but only the x509 certificates built into the kernel image should be > automatically trusted and can be added to a trusted keyring, because the > kernel itself was signed (and verified). These certificates extend the > (UEFI) certificate chain of trust that is rooted in hardware to the OS. That doesn't sound accurate to me. The cert built into the kernel image doesn't extend the UEFI certificates. In most cases, it is a ephemeral cert that is automatically generated at kernel build time and then discarded. It is not chained to or derived from any of the UEFI certs stored in the db (or mok) variables. The built-in cert is used for module loading verification. I agree that it should be trusted, but not really for the reason you list. Perhaps you meant the key that the PE image of the kernel is signed with? If so, the kernel doesn't load that. Only shim (and grub2 via shim) read that key. However, that does bring up the UEFI db/mok certs and how to deal with those. The out-of-tree patches we have add them to the system keyring as trusted keys. We can modify the patches to use KEY_ALLOC_TRUSTED to preserve that functionality I suppose. josh -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/5] firmware: generalize reading file contents as a helper
On Thu, Oct 8, 2015 at 6:54 PM, Luis R. Rodriguez wrote: > On Thu, Oct 08, 2015 at 01:36:53PM -0400, Josh Boyer wrote: >> On Thu, Oct 1, 2015 at 1:44 PM, Luis R. Rodriguez >> wrote: >> > From: David Howells >> > >> > We'll want to reuse this same code later in order to >> > read two separate types of file contents. This generalizes >> > fw_read_file() for reading a file rebrands it as fw_read_file(). >> >> Er, maybe that should read "...fw_read_file_contents() for reading a >> file and rebrands it as fw_read_file()." ? > > Thanks, corrected. > >> > This caller lets us pegs arbitrary data onto the target >> > buffer and size if the file is found. >> >> This sentence is somewhat confusing. The data isn't arbitrary. It is >> what the caller wants you to read from path. What is arbitrary, at >> least in the context of this function, is the path passed to it. >> Maybe rewrite this as: >> >> "The new function allows us to read file contents from arbitrary paths >> and return the data and size of the files read." > > The path is arbitrary but what I meant by arbitrary data is that > the data need no longer be firmware, whereas fw_read_file_contents() > *did* require passing firmware_class data structures. What this does > is it make the possibility of eventually making a more core system > data file reader more obvious, so for instance the goal is to later > share a reader with: > > - firmware_class: fw_read_file() > - module: kernel_read() > - kexec: copy_file_fd() > > I will clarify this in the commit log and also clarify the path is > arbitrary as well as you note. > >> > While at it this cleans up the exit paths on fw_read_file(). >> > >> > Signed-off-by: David Howells >> > Signed-off-by: Luis R. Rodriguez >> >> The code changes themselves look fine. > > Thank you for the review. Can I peg your Acked-by or Reviewed-by? > How about this for a change in the commit log: > > firmware: generalize reading file contents as a helper > > We'll want to reuse this same code later in order to read > two separate types of file contents. This generalizes > fw_read_file_contents() for reading a file and rebrands it > as fw_read_file(). This new caller is now generic and that > path can be arbitrary, the caller is also agnostic to the > firmware_class code now, which begs the possibility of code > re-use with other similar callers in the kernel. For instance > in the future we may want to share a solution with: > > - firmware_class: fw_read_file() > - module: kernel_read() > - kexec: copy_file_fd() > > While at it this also cleans up the exit paths on fw_read_file(). That reads much clearer to me. Thanks. With that changed: Reviewed-by: Josh Boyer josh -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] firmware: generalize "firmware" as "system data" helpers
On Tue, Oct 6, 2015 at 5:08 AM, Greg KH wrote: > Just responding to one thing at the moment: > > On Mon, Oct 05, 2015 at 11:22:22PM +0200, Luis R. Rodriguez wrote: >> * we should phase out the usermode helper from firmware_class long term > > You can "phase out", but you can not delete it as it's a user/kernel api > that we have to support for forever, sorry. Assuming that dell-rbu is the only in-tree legitimate user of the userhelper code, I'm curious if the code itself could simply move into that driver. It might help prevent the spread of reliance on an API we don't want to see grow in usage. We'd probably need to evaluate if the two new users could migrate off that. > Also, for some devices / use cases, the usermode helper is the solution > (think async loading of firmware when the host wants to do it.) Are any of those use cases in the kernel today, aside from dell-rbu? Would Luis' async mode to system data suffice? josh -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/5] firmware: add an extensible system data helpers
On Thu, Oct 1, 2015 at 1:44 PM, Luis R. Rodriguez
wrote:
> diff --git a/include/linux/sysdata.h b/include/linux/sysdata.h
> new file mode 100644
> index ..a69cf5ef082c
> --- /dev/null
> +++ b/include/linux/sysdata.h
> @@ -0,0 +1,208 @@
> +#ifndef _LINUX_SYSDATA_H
> +#define _LINUX_SYSDATA_H
> +
> +#include
> +#include
> +#include
> +
> +/*
> + * System Data internals
> + *
> + * Copyright (C) 2015 Luis R. Rodriguez
> + *
> + * 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,
Re: [PATCH v2 4/5] firmware: generalize reading file contents as a helper
On Thu, Oct 1, 2015 at 1:44 PM, Luis R. Rodriguez wrote: > From: David Howells > > We'll want to reuse this same code later in order to > read two separate types of file contents. This generalizes > fw_read_file() for reading a file rebrands it as fw_read_file(). Er, maybe that should read "...fw_read_file_contents() for reading a file and rebrands it as fw_read_file()." ? > This caller lets us pegs arbitrary data onto the target > buffer and size if the file is found. This sentence is somewhat confusing. The data isn't arbitrary. It is what the caller wants you to read from path. What is arbitrary, at least in the context of this function, is the path passed to it. Maybe rewrite this as: "The new function allows us to read file contents from arbitrary paths and return the data and size of the files read." > While at it this cleans up the exit paths on fw_read_file(). > > Signed-off-by: David Howells > Signed-off-by: Luis R. Rodriguez The code changes themselves look fine. josh -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
