Messages in this thread | | | Subject | Re: [PATCH] Add missing bound checks for software 842 decompressor | From | Joan Bruguera <> | Date | Sun, 16 Aug 2020 02:33:41 +0200 |
| |
Any feedback?
- Joan
On 05.06.20 17:44, Joan Bruguera wrote: > The software 842 decompressor receives, through the initial value of the > 'olen' parameter, the capacity of the buffer pointed to by 'out'. If this > capacity is insufficient to decode the compressed bitstream, -ENOSPC > should be returned. > > However, the bounds checks are missing for index references (for those > ops. where decomp_ops includes a I2, I4 or I8 subop.), and also for > OP_SHORT_DATA. Due to this, sw842_decompress can write past the capacity > of the 'out' buffer. > > The case for index references can be triggered by compressing data that > follows a 16-byte periodic pattern (excluding special cases which are > better encoded by OP_ZEROS) and passing a 'olen' somewhat smaller than the > original length. > The case for OP_SHORT_DATA can be triggered by compressing an amount of > data that is not a multiple of 8, and then a slightly smaller 'olen' that > can't fit those last <8 bytes. > > Following is a small test module to demonstrate the issue. > > - > > #include <linux/module.h> > #include <linux/kernel.h> > #include <linux/sw842.h> > > static unsigned char workspace[1000000] = { 0 }; // Hacky > > static void test_bound(const char *name, unsigned ibound, unsigned dbound) > { > uint8_t in[ibound], out[ibound * 4], decomp[ibound /* Overallocated */]; > unsigned clen = ibound * 4, dlen = dbound, i; > int ret; > > for (i = 0; i < ibound; i ++) > in[i] = i % 16; // 0, 1, 2, ..., 14, 15, 0, 1, 2, ... > for (i = dbound; i < ibound; i++) > decomp[i] = 0xFF; // Place guard bytes > > ret = sw842_compress(in, ibound, out, &clen, workspace); > BUG_ON(ret != 0); > > ret = sw842_decompress(out, clen, decomp, &dlen); > if (ret != -ENOSPC) { > printk(KERN_ERR "%s: Expected ENOSPC to be returned\n", name); > } > for (i = dbound; i < ibound; i++) { > if (decomp[i] != 0xFF) { > printk(KERN_ERR "%s: Guard overwritten\n", name); > break; > } > } > } > > int init_module(void) > { > test_bound("Index reference test", 256, 64); > test_bound("Short data test", 12, 8); > return -ECANCELED; // Do not leave this test module hanging around > } > > void cleanup_module(void) > { > } > > MODULE_LICENSE("GPL"); > MODULE_SOFTDEP("pre: 842"); > > Signed-off-by: Joan Bruguera <joanbrugueram@gmail.com> > --- > lib/842/842_decompress.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/lib/842/842_decompress.c b/lib/842/842_decompress.c > index 582085ef8b4..c29fbfc9d08 100644 > --- a/lib/842/842_decompress.c > +++ b/lib/842/842_decompress.c > @@ -202,6 +202,9 @@ static int __do_index(struct sw842_param *p, u8 size, u8 bits, u64 fsize) > (unsigned long)total, > (unsigned long)beN_to_cpu(&p->ostart[offset], size)); > > + if (size > p->olen) > + return -ENOSPC; > + > memcpy(p->out, &p->ostart[offset], size); > p->out += size; > p->olen -= size; > @@ -345,6 +348,9 @@ int sw842_decompress(const u8 *in, unsigned int ilen, > if (!bytes || bytes > SHORT_DATA_BITS_MAX) > return -EINVAL; > > + if (bytes > p.olen) > + return -ENOSPC; > + > while (bytes-- > 0) { > ret = next_bits(&p, &tmp, 8); > if (ret) >
| |