From: Joel Fernandes <joel@joelfernandes.org>
To: Kees Cook <keescook@chromium.org>
Cc: linux-kernel@vger.kernel.org, Anton Vorontsov <anton@enomsg.org>,
Colin Cross <ccross@android.com>, Tony Luck <tony.luck@intel.com>
Subject: Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression
Date: Fri, 2 Nov 2018 11:24:54 -0700 [thread overview]
Message-ID: <20181102182454.GB14942@google.com> (raw)
In-Reply-To: <20181101235200.28584-3-keescook@chromium.org>
On Thu, Nov 01, 2018 at 04:51:54PM -0700, Kees Cook wrote:
> The pre-allocated compression buffer used for crash dumping was also
> being used for decompression. This isn't technically safe, since it's
> possible the kernel may attempt a crashdump while pstore is populating the
> pstore filesystem (and performing decompression). Instead, just allocate
Yeah, that would be bad if it happened ;)
> a separate buffer for decompression. Correctness is preferred over
> performance here.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> fs/pstore/platform.c | 56 ++++++++++++++++++++------------------------
> 1 file changed, 25 insertions(+), 31 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index b821054ca3ed..8b6028948cf3 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -258,20 +258,6 @@ static int pstore_compress(const void *in, void *out,
> return outlen;
> }
>
> -static int pstore_decompress(void *in, void *out,
> - unsigned int inlen, unsigned int outlen)
> -{
> - int ret;
> -
> - ret = crypto_comp_decompress(tfm, in, inlen, out, &outlen);
> - if (ret) {
> - pr_err("crypto_comp_decompress failed, ret = %d!\n", ret);
> - return ret;
> - }
> -
> - return outlen;
> -}
> -
> static void allocate_buf_for_compression(void)
> {
> struct crypto_comp *ctx;
> @@ -656,8 +642,9 @@ EXPORT_SYMBOL_GPL(pstore_unregister);
>
> static void decompress_record(struct pstore_record *record)
> {
> + int ret;
> int unzipped_len;
nit: We could get rid of the unzipped_len variable now I think.
> - char *decompressed;
> + char *unzipped, *workspace;
>
> if (!record->compressed)
> return;
> @@ -668,35 +655,42 @@ static void decompress_record(struct pstore_record *record)
> return;
> }
>
> - /* No compression method has created the common buffer. */
> + /* Missing compression buffer means compression was not initialized. */
> if (!big_oops_buf) {
> - pr_warn("no decompression buffer allocated\n");
> + pr_warn("no decompression method initialized!\n");
> return;
> }
>
> - unzipped_len = pstore_decompress(record->buf, big_oops_buf,
> - record->size, big_oops_buf_sz);
> - if (unzipped_len <= 0) {
> - pr_err("decompression failed: %d\n", unzipped_len);
> + /* Allocate enough space to hold max decompression and ECC. */
> + unzipped_len = big_oops_buf_sz;
> + workspace = kmalloc(unzipped_len + record->ecc_notice_size,
Should tihs be unzipped_len + record->ecc_notice_size + 1. The extra byte
being for the NULL character of the ecc notice?
This occurred to me when I saw the + 1 in ram.c. It could be better to just
abstract the size as a macro.
> + GFP_KERNEL);
> + if (!workspace)
> return;
> - }
>
> - /* Build new buffer for decompressed contents. */
> - decompressed = kmalloc(unzipped_len + record->ecc_notice_size,
> - GFP_KERNEL);
> - if (!decompressed) {
> - pr_err("decompression ran out of memory\n");
> + /* After decompression "unzipped_len" is almost certainly smaller. */
> + ret = crypto_comp_decompress(tfm, record->buf, record->size,
> + workspace, &unzipped_len);
> + if (ret) {
> + pr_err("crypto_comp_decompress failed, ret = %d!\n", ret);
> + kfree(workspace);
> return;
> }
> - memcpy(decompressed, big_oops_buf, unzipped_len);
>
> /* Append ECC notice to decompressed buffer. */
> - memcpy(decompressed + unzipped_len, record->buf + record->size,
> + memcpy(workspace + unzipped_len, record->buf + record->size,
> record->ecc_notice_size);
>
> - /* Swap out compresed contents with decompressed contents. */
> + /* Copy decompressed contents into an minimum-sized allocation. */
> + unzipped = kmemdup(workspace, unzipped_len + record->ecc_notice_size,
> + GFP_KERNEL);
> + kfree(workspace);
> + if (!unzipped)
> + return;
> +
> + /* Swap out compressed contents with decompressed contents. */
> kfree(record->buf);
> - record->buf = decompressed;
> + record->buf = unzipped;
Rest of it LGTM, thanks!
- Joel
next prev parent reply other threads:[~2018-11-02 18:25 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-01 23:51 [PATCH 0/8] pstore improvements (pstore-next) Kees Cook
2018-11-01 23:51 ` [PATCH linux-next 1/8] pstore/ram: Standardize module name in ramoops Kees Cook
2018-11-01 23:51 ` [PATCH 2/8] pstore: Do not use crash buffer for decompression Kees Cook
2018-11-02 18:24 ` Joel Fernandes [this message]
2018-11-14 7:56 ` Kees Cook
2018-11-20 21:43 ` Joel Fernandes
2018-11-29 22:06 ` Kees Cook
2018-11-30 2:26 ` Joel Fernandes
2018-11-01 23:51 ` [PATCH 3/8] pstore/ram: Report backend assignments with finer granularity Kees Cook
2018-11-01 23:51 ` [PATCH 4/8] pstore/ram: Add kern-doc for struct persistent_ram_zone Kees Cook
2018-11-01 23:51 ` [PATCH 5/8] pstore: Improve and update some comments and status output Kees Cook
2018-11-01 23:51 ` [PATCH 6/8] pstore: Replace open-coded << with BIT() Kees Cook
2018-11-01 23:51 ` [PATCH 7/8] pstore: Remove needless lock during console writes Kees Cook
2018-11-02 18:32 ` Joel Fernandes
2018-11-02 20:40 ` Kees Cook
2018-11-02 21:50 ` Joel Fernandes
2018-11-01 23:52 ` [PATCH 8/8] pstore/ram: Correctly calculate usable PRZ bytes Kees Cook
2018-11-02 18:01 ` Joel Fernandes
2018-11-02 20:00 ` Kees Cook
2018-11-05 4:42 ` Joel Fernandes
2018-11-05 17:04 ` Kees Cook
2018-11-06 4:42 ` Joel Fernandes
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181102182454.GB14942@google.com \
--to=joel@joelfernandes.org \
--cc=anton@enomsg.org \
--cc=ccross@android.com \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tony.luck@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).