[PATCH 1/2] Revert "UBIFS: Fix potential integer overflow in allocation"

[PATCH 1/2] Revert "UBIFS: Fix potential integer overflow in allocation"

[Richard Weinberger]

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;
 
-- 
2.18.0

[PATCH 2/2] ubifs: Check data node size before truncate

[Richard Weinberger]

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);
-- 
2.18.0

Re: [PATCH 1/2] Revert "UBIFS: Fix potential integer overflow in allocation"

[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.

> 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

-- 
Kees Cook
Pixel Security

Re: [PATCH 2/2] ubifs: Check data node size before truncate

[Kees Cook]

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
>



-- 
Kees Cook
Pixel Security

Re: [PATCH 1/2] Revert "UBIFS: Fix potential integer overflow in allocation"

[Richard Weinberger]

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

Re: [PATCH 1/2] Revert "UBIFS: Fix potential integer overflow in allocation"

[Kees Cook]

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

-- 
Kees Cook
Pixel Security

Re: [PATCH 1/2] Revert "UBIFS: Fix potential integer overflow in allocation"

[Richard Weinberger]

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

Re: [PATCH 1/2] Revert "UBIFS: Fix potential integer overflow in allocation"

[Kees Cook]

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

-- 
Kees Cook
Pixel Security