Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression
On Thu, Nov 29, 2018 at 02:06:39PM -0800, Kees Cook wrote: > On Tue, Nov 13, 2018 at 11:56 PM Kees Cook wrote: > > On Fri, Nov 2, 2018 at 1:24 PM, Joel Fernandes > > wrote: > > > On Thu, Nov 01, 2018 at 04:51:54PM -0700, Kees Cook wrote: > > >> + 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. > > > > Ooh, yes, good catch. I'll get this fixed. > > I spent more time looking at this, and it seems that only the initial > creation of this string needs the +1, since all other operations are > byte-based not NUL-terminated string based. It's a big odd, and I > might try to clean it up differently, but as it stands, this is okay. > (See inode.c which doesn't include the trailing NUL byte.) Ok. Yes it does seem a bit inconsistent but I agree its not an issue for this particular patch. Sorry to waste your time, thanks! - Joel
Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression
On Thu, Nov 29, 2018 at 02:06:39PM -0800, Kees Cook wrote: > On Tue, Nov 13, 2018 at 11:56 PM Kees Cook wrote: > > On Fri, Nov 2, 2018 at 1:24 PM, Joel Fernandes > > wrote: > > > On Thu, Nov 01, 2018 at 04:51:54PM -0700, Kees Cook wrote: > > >> + 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. > > > > Ooh, yes, good catch. I'll get this fixed. > > I spent more time looking at this, and it seems that only the initial > creation of this string needs the +1, since all other operations are > byte-based not NUL-terminated string based. It's a big odd, and I > might try to clean it up differently, but as it stands, this is okay. > (See inode.c which doesn't include the trailing NUL byte.) Ok. Yes it does seem a bit inconsistent but I agree its not an issue for this particular patch. Sorry to waste your time, thanks! - Joel
Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression
On Tue, Nov 13, 2018 at 11:56 PM Kees Cook wrote: > On Fri, Nov 2, 2018 at 1:24 PM, Joel Fernandes wrote: > > On Thu, Nov 01, 2018 at 04:51:54PM -0700, Kees Cook wrote: > >> + 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. > > Ooh, yes, good catch. I'll get this fixed. I spent more time looking at this, and it seems that only the initial creation of this string needs the +1, since all other operations are byte-based not NUL-terminated string based. It's a big odd, and I might try to clean it up differently, but as it stands, this is okay. (See inode.c which doesn't include the trailing NUL byte.) -Kees -- Kees Cook
Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression
On Tue, Nov 13, 2018 at 11:56 PM Kees Cook wrote: > On Fri, Nov 2, 2018 at 1:24 PM, Joel Fernandes wrote: > > On Thu, Nov 01, 2018 at 04:51:54PM -0700, Kees Cook wrote: > >> + 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. > > Ooh, yes, good catch. I'll get this fixed. I spent more time looking at this, and it seems that only the initial creation of this string needs the +1, since all other operations are byte-based not NUL-terminated string based. It's a big odd, and I might try to clean it up differently, but as it stands, this is okay. (See inode.c which doesn't include the trailing NUL byte.) -Kees -- Kees Cook
Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression
On Wed, Nov 14, 2018 at 01:56:09AM -0600, Kees Cook wrote:
> On Fri, Nov 2, 2018 at 1:24 PM, Joel Fernandes wrote:
> > On Thu, Nov 01, 2018 at 04:51:54PM -0700, Kees Cook wrote:
> >> 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.
>
> I didn't follow this -- it gets used quite a bit. I don't see a clean
> way to remove it?
You are right. Sorry I missed that crpyto_comp_decompress actually uses it.
thanks,
- Joel
Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression
On Wed, Nov 14, 2018 at 01:56:09AM -0600, Kees Cook wrote:
> On Fri, Nov 2, 2018 at 1:24 PM, Joel Fernandes wrote:
> > On Thu, Nov 01, 2018 at 04:51:54PM -0700, Kees Cook wrote:
> >> 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.
>
> I didn't follow this -- it gets used quite a bit. I don't see a clean
> way to remove it?
You are right. Sorry I missed that crpyto_comp_decompress actually uses it.
thanks,
- Joel
Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression
On Fri, Nov 2, 2018 at 1:24 PM, Joel Fernandes wrote:
> On Thu, Nov 01, 2018 at 04:51:54PM -0700, Kees Cook wrote:
>> 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.
I didn't follow this -- it gets used quite a bit. I don't see a clean
way to remove it?
>> + 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.
Ooh, yes, good catch. I'll get this fixed.
Thanks for the review!
--
Kees Cook
Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression
On Fri, Nov 2, 2018 at 1:24 PM, Joel Fernandes wrote:
> On Thu, Nov 01, 2018 at 04:51:54PM -0700, Kees Cook wrote:
>> 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.
I didn't follow this -- it gets used quite a bit. I don't see a clean
way to remove it?
>> + 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.
Ooh, yes, good catch. I'll get this fixed.
Thanks for the review!
--
Kees Cook
Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression
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
> ---
> 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, );
> - 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, _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
Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression
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
> ---
> 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, );
> - 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, _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
[PATCH 2/8] pstore: Do not use crash buffer for decompression
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
a separate buffer for decompression. Correctness is preferred over
performance here.
Signed-off-by: Kees Cook
---
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, );
- 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;
- 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,
+ 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, _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;
record->size = unzipped_len;
record->compressed = false;
}
--
2.17.1
[PATCH 2/8] pstore: Do not use crash buffer for decompression
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
a separate buffer for decompression. Correctness is preferred over
performance here.
Signed-off-by: Kees Cook
---
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, );
- 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;
- 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,
+ 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, _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;
record->size = unzipped_len;
record->compressed = false;
}
--
2.17.1
