linux-kernel - Re: [RFC] LZO1X de/compression support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 18 May 2007 16:57:25 +0530
From:	"Nitin Gupta" <nitingupta910@...il.com>
To:	"Heikki Orsila" <shdl@...alwe.fi>
Cc:	linux-kernel@...r.kernel.org,
	"Richard Purdie" <richard@...nedhand.com>,
	"Markus F.X.J. Oberhumer" <markus@...rhumer.com>
Subject: Re: [RFC] LZO1X de/compression support

Hi,

Thanks for review. My comments inline.

On 5/18/07, Heikki Orsila <shdl@...alwe.fi> wrote:
> Good work..
>
> On Fri, May 18, 2007 at 03:28:31PM +0530, Nitin Gupta wrote:
> > Facts for LZO (at least for original code. Should hold true for this
> > port also - hence the RFC!):
> > - The compressor can never overrun buffer.
> > - The "non-safe" version of decompressor can never overrun buffer if
> > compressed data is unmodified. I am not sure about this if compressed
> > data is malicious (to be confirmed from the author).
> > - The "safe" version can never crash (buffer overrun etc.) - confirmed
> > from the author.
>
> What's the proof?

I confirmned these from the author - I just ported this code. I think
he can answer you better - CC'ed him  :-)

>
> > +/* LZO1X_1 compression */
> > +int
> > +lzo1x_compress(const unsigned char *src, size_t src_len,
> > +             unsigned char *dst, size_t *dst_len,
> > +             void *workmem);
>
> int lzo1x_compress(const unsigned char *src, size_t src_len,
>                    unsigned char *dst, size_t *dst_len,
>                    void *workmem);
>
> is the preferred style.
>

OK. Changed.

<snip>
> > +     register const unsigned char *ip;
>
> Is the register directive really useful? Or any subsequent usage of that
> directive?

The author must be having some performance gain with this directive.
Though I didn't test performance changes with/without this directive.

> > +             DINDEX1(dindex,ip);
>
> Put a space after the delimiter: DINDEX1(dindex, ip); This happens in
> many places in the source, fix them all.

OK.

> Useless brackets: (unsigned char) tt
>
OK.

> > +             }
> > +             do *op++ = *ii++; while (--t > 0);
>
> memcpy(op, ii, t); ? Happens in other places as well.

I looked more carefully into such cases. Following type of code blocks
are repeated at several places:

---
                COPY4(op,ip);
                op += 4;
                ip += 4;
                if (--t > 0) {
                        if (t >= 4) {
                                do {
                                        COPY4(op,ip);
                                        op += 4; ip += 4; t -= 4;
                                } while (t >= 4);
                                if (t > 0)
                                        do
                                                *op++ = *ip++;
                                        while (--t > 0);
                        } else
                                do
                                        *op++ = *ip++;
                                while (--t > 0);
                }
---
Such entire blocks can be replaced by simple:
memcpy(op, ip, t + 4);
Since kernel has separate memcpy() implementation optimized for
specific archs, we shouldn't loose on perf while having simpler (and
shorter) code.

I will work on this and post again.


> > +#define COPY4(dst,src)       *(uint32_t *)(dst) = *(uint32_t *)(src)
>
> Use u32.
>

What is the problem with uint32_t? Anyhow, I think COPY4 will
disappear after those memcpy changes :)

Thanks for comments. I will post revised patch soon.


Cheers,
Nitin
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/