This reverts commit 353748a359f1821ee934afc579cf04572406b420. It bypassed the linux-mtd review process and fixes the issue not as it should.
Cc: Kees Cook keescook@chromium.org Cc: Silvio Cesare silvio.cesare@gmail.com Cc: stable@vger.kernel.org Signed-off-by: Richard Weinberger richard@nod.at --- fs/ubifs/journal.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c index 07b4956e0425..da8afdfccaa6 100644 --- a/fs/ubifs/journal.c +++ b/fs/ubifs/journal.c @@ -1282,11 +1282,10 @@ static int truncate_data_node(const struct ubifs_info *c, const struct inode *in int *new_len) { void *buf; - int err, compr_type; - u32 dlen, out_len, old_dlen; + int err, dlen, compr_type, out_len, old_dlen;
out_len = le32_to_cpu(dn->size); - buf = kmalloc_array(out_len, WORST_COMPR_FACTOR, GFP_NOFS); + buf = kmalloc(out_len * WORST_COMPR_FACTOR, GFP_NOFS); if (!buf) return -ENOMEM;
Check whether the size is within bounds before using it. If the size is not correct, abort and dump the bad data node.
Cc: Kees Cook keescook@chromium.org Cc: Silvio Cesare silvio.cesare@gmail.com Cc: stable@vger.kernel.org Fixes: 1e51764a3c2ac ("UBIFS: add new flash file system") Reported-by: Silvio Cesare silvio.cesare@gmail.com Signed-off-by: Richard Weinberger richard@nod.at --- fs/ubifs/journal.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c index da8afdfccaa6..eea12d25a58b 100644 --- a/fs/ubifs/journal.c +++ b/fs/ubifs/journal.c @@ -1387,7 +1387,16 @@ int ubifs_jnl_truncate(struct ubifs_info *c, const struct inode *inode, else if (err) goto out_free; else { - if (le32_to_cpu(dn->size) <= dlen) + int dn_len = le32_to_cpu(dn->size); + + if (dn_len <= 0 || dn_len > UBIFS_BLOCK_SIZE) { + ubifs_err(c, "bad data node (block %u, inode %lu)", + blk, inode->i_ino); + ubifs_dump_node(c, dn); + goto out_free; + } + + if (dn_len <= dlen) dlen = 0; /* Nothing to do */ else { err = truncate_data_node(c, inode, blk, dn, &dlen);
On Sun, Jul 1, 2018 at 2:20 PM, Richard Weinberger richard@nod.at wrote:
Check whether the size is within bounds before using it. If the size is not correct, abort and dump the bad data node.
Cc: Kees Cook keescook@chromium.org Cc: Silvio Cesare silvio.cesare@gmail.com Cc: stable@vger.kernel.org Fixes: 1e51764a3c2ac ("UBIFS: add new flash file system") Reported-by: Silvio Cesare silvio.cesare@gmail.com Signed-off-by: Richard Weinberger richard@nod.at
Reviewed-by: Kees Cook keescook@chromium.org
-Kees
fs/ubifs/journal.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c index da8afdfccaa6..eea12d25a58b 100644 --- a/fs/ubifs/journal.c +++ b/fs/ubifs/journal.c @@ -1387,7 +1387,16 @@ int ubifs_jnl_truncate(struct ubifs_info *c, const struct inode *inode, else if (err) goto out_free; else {
if (le32_to_cpu(dn->size) <= dlen)
int dn_len = le32_to_cpu(dn->size);if (dn_len <= 0 || dn_len > UBIFS_BLOCK_SIZE) {ubifs_err(c, "bad data node (block %u, inode %lu)",blk, inode->i_ino);ubifs_dump_node(c, dn);goto out_free;}if (dn_len <= dlen) dlen = 0; /* Nothing to do */ else { err = truncate_data_node(c, inode, blk, dn, &dlen);-- 2.18.0
On Sun, Jul 1, 2018 at 2:20 PM, Richard Weinberger richard@nod.at wrote:
This reverts commit 353748a359f1821ee934afc579cf04572406b420. It bypassed the linux-mtd review process and fixes the issue not as it should.
Ah, sorry, I thought you were CCed on the original report.
Cc: Kees Cook keescook@chromium.org Cc: Silvio Cesare silvio.cesare@gmail.com Cc: stable@vger.kernel.org Signed-off-by: Richard Weinberger richard@nod.at
fs/ubifs/journal.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c index 07b4956e0425..da8afdfccaa6 100644 --- a/fs/ubifs/journal.c +++ b/fs/ubifs/journal.c @@ -1282,11 +1282,10 @@ static int truncate_data_node(const struct ubifs_info *c, const struct inode *in int *new_len) { void *buf;
int err, compr_type;u32 dlen, out_len, old_dlen;
int err, dlen, compr_type, out_len, old_dlen;
What's wrong with making these unsigned?
out_len = le32_to_cpu(dn->size);
buf = kmalloc_array(out_len, WORST_COMPR_FACTOR, GFP_NOFS);
buf = kmalloc(out_len * WORST_COMPR_FACTOR, GFP_NOFS); if (!buf) return -ENOMEM;
Please leave the kmalloc() -> kmalloc_array() change, as that has happened treewide already. We don't want to have any multiplications in the size argument for the allocators (i.e. they should use 2-factor arg version like here, or use array_size() for things like vmalloc()).
Thanks!
-Kees
Am Montag, 2. Juli 2018, 18:00:05 CEST schrieb Kees Cook:
On Sun, Jul 1, 2018 at 2:20 PM, Richard Weinberger richard@nod.at wrote:
This reverts commit 353748a359f1821ee934afc579cf04572406b420. It bypassed the linux-mtd review process and fixes the issue not as it should.
Ah, sorry, I thought you were CCed on the original report.
No big deal. I was just "surprised".
Cc: Kees Cook keescook@chromium.org Cc: Silvio Cesare silvio.cesare@gmail.com Cc: stable@vger.kernel.org Signed-off-by: Richard Weinberger richard@nod.at
fs/ubifs/journal.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c index 07b4956e0425..da8afdfccaa6 100644 --- a/fs/ubifs/journal.c +++ b/fs/ubifs/journal.c @@ -1282,11 +1282,10 @@ static int truncate_data_node(const struct ubifs_info *c, const struct inode *in int *new_len) { void *buf;
int err, compr_type;u32 dlen, out_len, old_dlen;
int err, dlen, compr_type, out_len, old_dlen;What's wrong with making these unsigned?
Well, what is the benefit? In ubifs a data node carries at most 4k of bytes. WORST_COMPR_FACTOR is 2. So the computed lengths are always in a range where a natural int does work just fine.
out_len = le32_to_cpu(dn->size);
buf = kmalloc_array(out_len, WORST_COMPR_FACTOR, GFP_NOFS);
buf = kmalloc(out_len * WORST_COMPR_FACTOR, GFP_NOFS); if (!buf) return -ENOMEM;Please leave the kmalloc() -> kmalloc_array() change, as that has happened treewide already. We don't want to have any multiplications in the size argument for the allocators (i.e. they should use 2-factor arg version like here, or use array_size() for things like vmalloc()).
Let's queue another patch for the next merge window which converts kmalloc() -> kmalloc_array().
Thanks, //richard
On Mon, Jul 2, 2018 at 10:50 AM, Richard Weinberger richard@nod.at wrote:
Am Montag, 2. Juli 2018, 18:00:05 CEST schrieb Kees Cook:
On Sun, Jul 1, 2018 at 2:20 PM, Richard Weinberger richard@nod.at wrote:
This reverts commit 353748a359f1821ee934afc579cf04572406b420. It bypassed the linux-mtd review process and fixes the issue not as it should.
Ah, sorry, I thought you were CCed on the original report.
No big deal. I was just "surprised".
Yeah, totally my mistake. There were other overflow patches that went out pubically and I thought this one had too.
Cc: Kees Cook keescook@chromium.org Cc: Silvio Cesare silvio.cesare@gmail.com Cc: stable@vger.kernel.org Signed-off-by: Richard Weinberger richard@nod.at
fs/ubifs/journal.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c index 07b4956e0425..da8afdfccaa6 100644 --- a/fs/ubifs/journal.c +++ b/fs/ubifs/journal.c @@ -1282,11 +1282,10 @@ static int truncate_data_node(const struct ubifs_info *c, const struct inode *in int *new_len) { void *buf;
int err, compr_type;u32 dlen, out_len, old_dlen;
int err, dlen, compr_type, out_len, old_dlen;What's wrong with making these unsigned?
Well, what is the benefit? In ubifs a data node carries at most 4k of bytes. WORST_COMPR_FACTOR is 2. So the computed lengths are always in a range where a natural int does work just fine.
Just a robustness preference: it keeps it from going negative. But I don't feel strongly. :)
out_len = le32_to_cpu(dn->size);
buf = kmalloc_array(out_len, WORST_COMPR_FACTOR, GFP_NOFS);
buf = kmalloc(out_len * WORST_COMPR_FACTOR, GFP_NOFS); if (!buf) return -ENOMEM;Please leave the kmalloc() -> kmalloc_array() change, as that has happened treewide already. We don't want to have any multiplications in the size argument for the allocators (i.e. they should use 2-factor arg version like here, or use array_size() for things like vmalloc()).
Let's queue another patch for the next merge window which converts kmalloc() -> kmalloc_array().
I'd prefer to leave it as-is for 4.18 because it would be the only unconverted kmalloc()-with-multiplication in the entire tree. We did treewide conversions and a revert would be undoing that here. (The scripts that check for this case would run "clean" for 4.18.)
So, this gets back to the question of the int vs u32: if you just didn't revert this patch, then the kmalloc_array() would stand too. Easy! :)
-Kees
Am Montag, 2. Juli 2018, 20:27:00 CEST schrieb Kees Cook:
Let's queue another patch for the next merge window which converts kmalloc() -> kmalloc_array().
I'd prefer to leave it as-is for 4.18 because it would be the only unconverted kmalloc()-with-multiplication in the entire tree. We did treewide conversions and a revert would be undoing that here. (The scripts that check for this case would run "clean" for 4.18.)
So, this gets back to the question of the int vs u32: if you just didn't revert this patch, then the kmalloc_array() would stand too. Easy! :)
I can queue the kmalloc_array() conversion on top of the revert. But TBH, using kmalloc_array() here is just ridiculous, we allocate dn->size times 2 where dn->size is at most 4k.
Thanks, //richard
On Mon, Jul 2, 2018 at 2:41 PM, Richard Weinberger richard@nod.at wrote:
Am Montag, 2. Juli 2018, 20:27:00 CEST schrieb Kees Cook:
Let's queue another patch for the next merge window which converts kmalloc() -> kmalloc_array().
I'd prefer to leave it as-is for 4.18 because it would be the only unconverted kmalloc()-with-multiplication in the entire tree. We did treewide conversions and a revert would be undoing that here. (The scripts that check for this case would run "clean" for 4.18.)
So, this gets back to the question of the int vs u32: if you just didn't revert this patch, then the kmalloc_array() would stand too. Easy! :)
I can queue the kmalloc_array() conversion on top of the revert. But TBH, using kmalloc_array() here is just ridiculous, we allocate dn->size times 2 where dn->size is at most 4k.
Right, I don't think this spot still suddenly become vulnerable again, but it'll generate the same machine code (since one arg is a constant value), and then static checkers never have to flag on it again. :)
Thanks!
-Kees
linux-stable-mirror@lists.linaro.org
-
Kees Cook -
Richard Weinberger