| Message ID | 20180701212051.29486-1-richard@nod.at |
|---|---|
| State | Accepted |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | [1/2] Revert "UBIFS: Fix potential integer overflow in allocation" | expand |
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
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;
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(-)