* [PATCH 0/2] lzo: refine overrun checking
@ 2014-11-03 14:47 Jan Beulich
2014-11-03 15:01 ` [PATCH 1/2] Revert "lzo: properly check for overruns" Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jan Beulich @ 2014-11-03 14:47 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Campbell, Keir Fraser, Ian Jackson, Tim Deegan
The original fix we inherited from Linux go re-done. Let's do so too.
1: Revert "lzo: properly check for overruns"
2: lzo: check for length overrun in variable length encoding
Signed-off-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] Revert "lzo: properly check for overruns"
2014-11-03 14:47 [PATCH 0/2] lzo: refine overrun checking Jan Beulich
@ 2014-11-03 15:01 ` Jan Beulich
2014-11-03 15:02 ` [PATCH 2/2] lzo: check for length overrun in variable length encoding Jan Beulich
2014-11-04 9:37 ` [PATCH 0/2] lzo: refine overrun checking Ian Campbell
2 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2014-11-03 15:01 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Campbell, Keir Fraser, Ian Jackson, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 6064 bytes --]
This reverts commit 504f70b6 ("lzo: properly check for overruns").
As analysed by Willem Pinckaers, this fix is still incomplete on
certain rare corner cases, and it is easier to restart from the
original code.
Reported-by: Willem Pinckaers <willem@lekkertech.net>
Signed-off-by: Willy Tarreau <w@1wt.eu>
[original Linux commit: af958a38]
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/lzo.c
+++ b/xen/common/lzo.c
@@ -375,31 +375,11 @@ int lzo1x_1_compress(const unsigned char
* Richard Purdie <rpurdie@openedhand.com>
*/
-#define HAVE_IP(t, x) \
- (((size_t)(ip_end - ip) >= (size_t)(t + x)) && \
- (((t + x) >= t) && ((t + x) >= x)))
-
-#define HAVE_OP(t, x) \
- (((size_t)(op_end - op) >= (size_t)(t + x)) && \
- (((t + x) >= t) && ((t + x) >= x)))
-
-#define NEED_IP(t, x) \
- do { \
- if (!HAVE_IP(t, x)) \
- goto input_overrun; \
- } while (0)
-
-#define NEED_OP(t, x) \
- do { \
- if (!HAVE_OP(t, x)) \
- goto output_overrun; \
- } while (0)
-
-#define TEST_LB(m_pos) \
- do { \
- if ((m_pos) < out) \
- goto lookbehind_overrun; \
- } while (0)
+#define HAVE_IP(x) ((size_t)(ip_end - ip) >= (size_t)(x))
+#define HAVE_OP(x) ((size_t)(op_end - op) >= (size_t)(x))
+#define NEED_IP(x) if (!HAVE_IP(x)) goto input_overrun
+#define NEED_OP(x) if (!HAVE_OP(x)) goto output_overrun
+#define TEST_LB(m_pos) if ((m_pos) < out) goto lookbehind_overrun
int lzo1x_decompress_safe(const unsigned char *in, size_t in_len,
unsigned char *out, size_t *out_len)
@@ -434,14 +414,14 @@ int lzo1x_decompress_safe(const unsigned
while (unlikely(*ip == 0)) {
t += 255;
ip++;
- NEED_IP(1, 0);
+ NEED_IP(1);
}
t += 15 + *ip++;
}
t += 3;
copy_literal_run:
#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
- if (likely(HAVE_IP(t, 15) && HAVE_OP(t, 15))) {
+ if (likely(HAVE_IP(t + 15) && HAVE_OP(t + 15))) {
const unsigned char *ie = ip + t;
unsigned char *oe = op + t;
do {
@@ -457,8 +437,8 @@ int lzo1x_decompress_safe(const unsigned
} else
#endif
{
- NEED_OP(t, 0);
- NEED_IP(t, 3);
+ NEED_OP(t);
+ NEED_IP(t + 3);
do {
*op++ = *ip++;
} while (--t > 0);
@@ -471,7 +451,7 @@ int lzo1x_decompress_safe(const unsigned
m_pos -= t >> 2;
m_pos -= *ip++ << 2;
TEST_LB(m_pos);
- NEED_OP(2, 0);
+ NEED_OP(2);
op[0] = m_pos[0];
op[1] = m_pos[1];
op += 2;
@@ -495,10 +475,10 @@ int lzo1x_decompress_safe(const unsigned
while (unlikely(*ip == 0)) {
t += 255;
ip++;
- NEED_IP(1, 0);
+ NEED_IP(1);
}
t += 31 + *ip++;
- NEED_IP(2, 0);
+ NEED_IP(2);
}
m_pos = op - 1;
next = get_unaligned_le16(ip);
@@ -513,10 +493,10 @@ int lzo1x_decompress_safe(const unsigned
while (unlikely(*ip == 0)) {
t += 255;
ip++;
- NEED_IP(1, 0);
+ NEED_IP(1);
}
t += 7 + *ip++;
- NEED_IP(2, 0);
+ NEED_IP(2);
}
next = get_unaligned_le16(ip);
ip += 2;
@@ -530,7 +510,7 @@ int lzo1x_decompress_safe(const unsigned
#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
if (op - m_pos >= 8) {
unsigned char *oe = op + t;
- if (likely(HAVE_OP(t, 15))) {
+ if (likely(HAVE_OP(t + 15))) {
do {
COPY8(op, m_pos);
op += 8;
@@ -540,7 +520,7 @@ int lzo1x_decompress_safe(const unsigned
m_pos += 8;
} while (op < oe);
op = oe;
- if (HAVE_IP(6, 0)) {
+ if (HAVE_IP(6)) {
state = next;
COPY4(op, ip);
op += next;
@@ -548,7 +528,7 @@ int lzo1x_decompress_safe(const unsigned
continue;
}
} else {
- NEED_OP(t, 0);
+ NEED_OP(t);
do {
*op++ = *m_pos++;
} while (op < oe);
@@ -557,7 +537,7 @@ int lzo1x_decompress_safe(const unsigned
#endif
{
unsigned char *oe = op + t;
- NEED_OP(t, 0);
+ NEED_OP(t);
op[0] = m_pos[0];
op[1] = m_pos[1];
op += 2;
@@ -570,15 +550,15 @@ int lzo1x_decompress_safe(const unsigned
state = next;
t = next;
#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
- if (likely(HAVE_IP(6, 0) && HAVE_OP(4, 0))) {
+ if (likely(HAVE_IP(6) && HAVE_OP(4))) {
COPY4(op, ip);
op += t;
ip += t;
} else
#endif
{
- NEED_IP(t, 3);
- NEED_OP(t, 0);
+ NEED_IP(t + 3);
+ NEED_OP(t);
while (t > 0) {
*op++ = *ip++;
t--;
[-- Attachment #2: lzo-revert-504f70b6.patch --]
[-- Type: text/plain, Size: 6105 bytes --]
Revert "lzo: properly check for overruns"
This reverts commit 504f70b6 ("lzo: properly check for overruns").
As analysed by Willem Pinckaers, this fix is still incomplete on
certain rare corner cases, and it is easier to restart from the
original code.
Reported-by: Willem Pinckaers <willem@lekkertech.net>
Signed-off-by: Willy Tarreau <w@1wt.eu>
[original Linux commit: af958a38]
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/lzo.c
+++ b/xen/common/lzo.c
@@ -375,31 +375,11 @@ int lzo1x_1_compress(const unsigned char
* Richard Purdie <rpurdie@openedhand.com>
*/
-#define HAVE_IP(t, x) \
- (((size_t)(ip_end - ip) >= (size_t)(t + x)) && \
- (((t + x) >= t) && ((t + x) >= x)))
-
-#define HAVE_OP(t, x) \
- (((size_t)(op_end - op) >= (size_t)(t + x)) && \
- (((t + x) >= t) && ((t + x) >= x)))
-
-#define NEED_IP(t, x) \
- do { \
- if (!HAVE_IP(t, x)) \
- goto input_overrun; \
- } while (0)
-
-#define NEED_OP(t, x) \
- do { \
- if (!HAVE_OP(t, x)) \
- goto output_overrun; \
- } while (0)
-
-#define TEST_LB(m_pos) \
- do { \
- if ((m_pos) < out) \
- goto lookbehind_overrun; \
- } while (0)
+#define HAVE_IP(x) ((size_t)(ip_end - ip) >= (size_t)(x))
+#define HAVE_OP(x) ((size_t)(op_end - op) >= (size_t)(x))
+#define NEED_IP(x) if (!HAVE_IP(x)) goto input_overrun
+#define NEED_OP(x) if (!HAVE_OP(x)) goto output_overrun
+#define TEST_LB(m_pos) if ((m_pos) < out) goto lookbehind_overrun
int lzo1x_decompress_safe(const unsigned char *in, size_t in_len,
unsigned char *out, size_t *out_len)
@@ -434,14 +414,14 @@ int lzo1x_decompress_safe(const unsigned
while (unlikely(*ip == 0)) {
t += 255;
ip++;
- NEED_IP(1, 0);
+ NEED_IP(1);
}
t += 15 + *ip++;
}
t += 3;
copy_literal_run:
#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
- if (likely(HAVE_IP(t, 15) && HAVE_OP(t, 15))) {
+ if (likely(HAVE_IP(t + 15) && HAVE_OP(t + 15))) {
const unsigned char *ie = ip + t;
unsigned char *oe = op + t;
do {
@@ -457,8 +437,8 @@ int lzo1x_decompress_safe(const unsigned
} else
#endif
{
- NEED_OP(t, 0);
- NEED_IP(t, 3);
+ NEED_OP(t);
+ NEED_IP(t + 3);
do {
*op++ = *ip++;
} while (--t > 0);
@@ -471,7 +451,7 @@ int lzo1x_decompress_safe(const unsigned
m_pos -= t >> 2;
m_pos -= *ip++ << 2;
TEST_LB(m_pos);
- NEED_OP(2, 0);
+ NEED_OP(2);
op[0] = m_pos[0];
op[1] = m_pos[1];
op += 2;
@@ -495,10 +475,10 @@ int lzo1x_decompress_safe(const unsigned
while (unlikely(*ip == 0)) {
t += 255;
ip++;
- NEED_IP(1, 0);
+ NEED_IP(1);
}
t += 31 + *ip++;
- NEED_IP(2, 0);
+ NEED_IP(2);
}
m_pos = op - 1;
next = get_unaligned_le16(ip);
@@ -513,10 +493,10 @@ int lzo1x_decompress_safe(const unsigned
while (unlikely(*ip == 0)) {
t += 255;
ip++;
- NEED_IP(1, 0);
+ NEED_IP(1);
}
t += 7 + *ip++;
- NEED_IP(2, 0);
+ NEED_IP(2);
}
next = get_unaligned_le16(ip);
ip += 2;
@@ -530,7 +510,7 @@ int lzo1x_decompress_safe(const unsigned
#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
if (op - m_pos >= 8) {
unsigned char *oe = op + t;
- if (likely(HAVE_OP(t, 15))) {
+ if (likely(HAVE_OP(t + 15))) {
do {
COPY8(op, m_pos);
op += 8;
@@ -540,7 +520,7 @@ int lzo1x_decompress_safe(const unsigned
m_pos += 8;
} while (op < oe);
op = oe;
- if (HAVE_IP(6, 0)) {
+ if (HAVE_IP(6)) {
state = next;
COPY4(op, ip);
op += next;
@@ -548,7 +528,7 @@ int lzo1x_decompress_safe(const unsigned
continue;
}
} else {
- NEED_OP(t, 0);
+ NEED_OP(t);
do {
*op++ = *m_pos++;
} while (op < oe);
@@ -557,7 +537,7 @@ int lzo1x_decompress_safe(const unsigned
#endif
{
unsigned char *oe = op + t;
- NEED_OP(t, 0);
+ NEED_OP(t);
op[0] = m_pos[0];
op[1] = m_pos[1];
op += 2;
@@ -570,15 +550,15 @@ int lzo1x_decompress_safe(const unsigned
state = next;
t = next;
#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
- if (likely(HAVE_IP(6, 0) && HAVE_OP(4, 0))) {
+ if (likely(HAVE_IP(6) && HAVE_OP(4))) {
COPY4(op, ip);
op += t;
ip += t;
} else
#endif
{
- NEED_IP(t, 3);
- NEED_OP(t, 0);
+ NEED_IP(t + 3);
+ NEED_OP(t);
while (t > 0) {
*op++ = *ip++;
t--;
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] lzo: check for length overrun in variable length encoding
2014-11-03 14:47 [PATCH 0/2] lzo: refine overrun checking Jan Beulich
2014-11-03 15:01 ` [PATCH 1/2] Revert "lzo: properly check for overruns" Jan Beulich
@ 2014-11-03 15:02 ` Jan Beulich
2014-11-04 11:24 ` Ian Jackson
2014-11-04 9:37 ` [PATCH 0/2] lzo: refine overrun checking Ian Campbell
2 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2014-11-03 15:02 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Campbell, Keir Fraser, Ian Jackson, Tim Deegan
[-- Attachment #1: Type: text/plain, Size: 4664 bytes --]
This fix ensures that we never meet an integer overflow while adding
255 while parsing a variable length encoding. It works differently from
commit 504f70b6 ("lzo: properly check for overruns") because instead of
ensuring that we don't overrun the input, which is tricky to guarantee
due to many assumptions in the code, it simply checks that the cumulated
number of 255 read cannot overflow by bounding this number.
The MAX_255_COUNT is the maximum number of times we can add 255 to a base
count without overflowing an integer. The multiply will overflow when
multiplying 255 by more than MAXINT/255. The sum will overflow earlier
depending on the base count. Since the base count is taken from a u8
and a few bits, it is safe to assume that it will always be lower than
or equal to 2*255, thus we can always prevent any overflow by accepting
two less 255 steps.
This patch also reduces the CPU overhead and actually increases performance
by 1.1% compared to the initial code, while the previous fix costs 3.1%
(measured on x86_64).
The fix needs to be backported to all currently supported stable kernels.
Reported-by: Willem Pinckaers <willem@lekkertech.net>
Signed-off-by: Willy Tarreau <w@1wt.eu>
[original Linux commit: 72cf9012]
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/lzo.c
+++ b/xen/common/lzo.c
@@ -381,6 +381,16 @@ int lzo1x_1_compress(const unsigned char
#define NEED_OP(x) if (!HAVE_OP(x)) goto output_overrun
#define TEST_LB(m_pos) if ((m_pos) < out) goto lookbehind_overrun
+/* This MAX_255_COUNT is the maximum number of times we can add 255 to a base
+ * count without overflowing an integer. The multiply will overflow when
+ * multiplying 255 by more than MAXINT/255. The sum will overflow earlier
+ * depending on the base count. Since the base count is taken from a u8
+ * and a few bits, it is safe to assume that it will always be lower than
+ * or equal to 2*255, thus we can always prevent any overflow by accepting
+ * two less 255 steps. See Documentation/lzo.txt for more information.
+ */
+#define MAX_255_COUNT ((((size_t)~0) / 255) - 2)
+
int lzo1x_decompress_safe(const unsigned char *in, size_t in_len,
unsigned char *out, size_t *out_len)
{
@@ -411,12 +421,19 @@ int lzo1x_decompress_safe(const unsigned
if (t < 16) {
if (likely(state == 0)) {
if (unlikely(t == 0)) {
+ size_t offset;
+ const unsigned char *ip_last = ip;
+
while (unlikely(*ip == 0)) {
- t += 255;
ip++;
NEED_IP(1);
}
- t += 15 + *ip++;
+ offset = ip - ip_last;
+ if (unlikely(offset > MAX_255_COUNT))
+ return LZO_E_ERROR;
+
+ offset = (offset << 8) - offset;
+ t += offset + 15 + *ip++;
}
t += 3;
copy_literal_run:
@@ -472,12 +489,19 @@ int lzo1x_decompress_safe(const unsigned
} else if (t >= 32) {
t = (t & 31) + (3 - 1);
if (unlikely(t == 2)) {
+ size_t offset;
+ const unsigned char *ip_last = ip;
+
while (unlikely(*ip == 0)) {
- t += 255;
ip++;
NEED_IP(1);
}
- t += 31 + *ip++;
+ offset = ip - ip_last;
+ if (unlikely(offset > MAX_255_COUNT))
+ return LZO_E_ERROR;
+
+ offset = (offset << 8) - offset;
+ t += offset + 31 + *ip++;
NEED_IP(2);
}
m_pos = op - 1;
@@ -490,12 +514,19 @@ int lzo1x_decompress_safe(const unsigned
m_pos -= (t & 8) << 11;
t = (t & 7) + (3 - 1);
if (unlikely(t == 2)) {
+ size_t offset;
+ const unsigned char *ip_last = ip;
+
while (unlikely(*ip == 0)) {
- t += 255;
ip++;
NEED_IP(1);
}
- t += 7 + *ip++;
+ offset = ip - ip_last;
+ if (unlikely(offset > MAX_255_COUNT))
+ return LZO_E_ERROR;
+
+ offset = (offset << 8) - offset;
+ t += offset + 7 + *ip++;
NEED_IP(2);
}
next = get_unaligned_le16(ip);
[-- Attachment #2: lzo-check-length-overrun.patch --]
[-- Type: text/plain, Size: 4721 bytes --]
lzo: check for length overrun in variable length encoding
This fix ensures that we never meet an integer overflow while adding
255 while parsing a variable length encoding. It works differently from
commit 504f70b6 ("lzo: properly check for overruns") because instead of
ensuring that we don't overrun the input, which is tricky to guarantee
due to many assumptions in the code, it simply checks that the cumulated
number of 255 read cannot overflow by bounding this number.
The MAX_255_COUNT is the maximum number of times we can add 255 to a base
count without overflowing an integer. The multiply will overflow when
multiplying 255 by more than MAXINT/255. The sum will overflow earlier
depending on the base count. Since the base count is taken from a u8
and a few bits, it is safe to assume that it will always be lower than
or equal to 2*255, thus we can always prevent any overflow by accepting
two less 255 steps.
This patch also reduces the CPU overhead and actually increases performance
by 1.1% compared to the initial code, while the previous fix costs 3.1%
(measured on x86_64).
The fix needs to be backported to all currently supported stable kernels.
Reported-by: Willem Pinckaers <willem@lekkertech.net>
Signed-off-by: Willy Tarreau <w@1wt.eu>
[original Linux commit: 72cf9012]
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/lzo.c
+++ b/xen/common/lzo.c
@@ -381,6 +381,16 @@ int lzo1x_1_compress(const unsigned char
#define NEED_OP(x) if (!HAVE_OP(x)) goto output_overrun
#define TEST_LB(m_pos) if ((m_pos) < out) goto lookbehind_overrun
+/* This MAX_255_COUNT is the maximum number of times we can add 255 to a base
+ * count without overflowing an integer. The multiply will overflow when
+ * multiplying 255 by more than MAXINT/255. The sum will overflow earlier
+ * depending on the base count. Since the base count is taken from a u8
+ * and a few bits, it is safe to assume that it will always be lower than
+ * or equal to 2*255, thus we can always prevent any overflow by accepting
+ * two less 255 steps. See Documentation/lzo.txt for more information.
+ */
+#define MAX_255_COUNT ((((size_t)~0) / 255) - 2)
+
int lzo1x_decompress_safe(const unsigned char *in, size_t in_len,
unsigned char *out, size_t *out_len)
{
@@ -411,12 +421,19 @@ int lzo1x_decompress_safe(const unsigned
if (t < 16) {
if (likely(state == 0)) {
if (unlikely(t == 0)) {
+ size_t offset;
+ const unsigned char *ip_last = ip;
+
while (unlikely(*ip == 0)) {
- t += 255;
ip++;
NEED_IP(1);
}
- t += 15 + *ip++;
+ offset = ip - ip_last;
+ if (unlikely(offset > MAX_255_COUNT))
+ return LZO_E_ERROR;
+
+ offset = (offset << 8) - offset;
+ t += offset + 15 + *ip++;
}
t += 3;
copy_literal_run:
@@ -472,12 +489,19 @@ int lzo1x_decompress_safe(const unsigned
} else if (t >= 32) {
t = (t & 31) + (3 - 1);
if (unlikely(t == 2)) {
+ size_t offset;
+ const unsigned char *ip_last = ip;
+
while (unlikely(*ip == 0)) {
- t += 255;
ip++;
NEED_IP(1);
}
- t += 31 + *ip++;
+ offset = ip - ip_last;
+ if (unlikely(offset > MAX_255_COUNT))
+ return LZO_E_ERROR;
+
+ offset = (offset << 8) - offset;
+ t += offset + 31 + *ip++;
NEED_IP(2);
}
m_pos = op - 1;
@@ -490,12 +514,19 @@ int lzo1x_decompress_safe(const unsigned
m_pos -= (t & 8) << 11;
t = (t & 7) + (3 - 1);
if (unlikely(t == 2)) {
+ size_t offset;
+ const unsigned char *ip_last = ip;
+
while (unlikely(*ip == 0)) {
- t += 255;
ip++;
NEED_IP(1);
}
- t += 7 + *ip++;
+ offset = ip - ip_last;
+ if (unlikely(offset > MAX_255_COUNT))
+ return LZO_E_ERROR;
+
+ offset = (offset << 8) - offset;
+ t += offset + 7 + *ip++;
NEED_IP(2);
}
next = get_unaligned_le16(ip);
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] lzo: refine overrun checking
2014-11-03 14:47 [PATCH 0/2] lzo: refine overrun checking Jan Beulich
2014-11-03 15:01 ` [PATCH 1/2] Revert "lzo: properly check for overruns" Jan Beulich
2014-11-03 15:02 ` [PATCH 2/2] lzo: check for length overrun in variable length encoding Jan Beulich
@ 2014-11-04 9:37 ` Ian Campbell
2 siblings, 0 replies; 6+ messages in thread
From: Ian Campbell @ 2014-11-04 9:37 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Ian Jackson, Tim Deegan
On Mon, 2014-11-03 at 14:47 +0000, Jan Beulich wrote:
> The original fix we inherited from Linux go re-done. Let's do so too.
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Mainly on the basis that it was reviewed by Linux maintainers and then
by you rather than because I've looked at it closely myself.
Ian.
>
> 1: Revert "lzo: properly check for overruns"
> 2: lzo: check for length overrun in variable length encoding
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] lzo: check for length overrun in variable length encoding
2014-11-03 15:02 ` [PATCH 2/2] lzo: check for length overrun in variable length encoding Jan Beulich
@ 2014-11-04 11:24 ` Ian Jackson
2014-11-04 11:35 ` Ian Campbell
0 siblings, 1 reply; 6+ messages in thread
From: Ian Jackson @ 2014-11-04 11:24 UTC (permalink / raw)
To: Jan Beulich; +Cc: Ian Campbell, xen-devel, Keir Fraser, Tim Deegan
Jan Beulich writes ("[PATCH 2/2] lzo: check for length overrun in variable length encoding"):
> This fix ensures that we never meet an integer overflow while adding
> 255 while parsing a variable length encoding. It works differently from
> commit 504f70b6 ("lzo: properly check for overruns") because instead of
> ensuring that we don't overrun the input, which is tricky to guarantee
> due to many assumptions in the code, it simply checks that the cumulated
> number of 255 read cannot overflow by bounding this number.
AFAICT this decompressor is exposed to untrusted guest kernel images.
Ian.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] lzo: check for length overrun in variable length encoding
2014-11-04 11:24 ` Ian Jackson
@ 2014-11-04 11:35 ` Ian Campbell
0 siblings, 0 replies; 6+ messages in thread
From: Ian Campbell @ 2014-11-04 11:35 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Keir Fraser, Jan Beulich, Tim Deegan
On Tue, 2014-11-04 at 11:24 +0000, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH 2/2] lzo: check for length overrun in variable length encoding"):
> > This fix ensures that we never meet an integer overflow while adding
> > 255 while parsing a variable length encoding. It works differently from
> > commit 504f70b6 ("lzo: properly check for overruns") because instead of
> > ensuring that we don't overrun the input, which is tricky to guarantee
> > due to many assumptions in the code, it simply checks that the cumulated
> > number of 255 read cannot overflow by bounding this number.
>
> AFAICT this decompressor is exposed to untrusted guest kernel images.
Only in mini-os context, I think. AIUI dom0 hosted libxc uses liblzo2.
Not 100% sure of that, but tools/libxc/Makefile's handling of
xc_dom_decompress_unsafe_lzo1x seems to confirm what I thought.
Aside from that, I presume you are trying to say that the description of
the fix suggests the code would be vulnerable to untrusted input? It
looks to me as if it is infact OK for untrusted input, but perhaps I've
misunderstood what it is doing.
Ian.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-11-04 11:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-03 14:47 [PATCH 0/2] lzo: refine overrun checking Jan Beulich
2014-11-03 15:01 ` [PATCH 1/2] Revert "lzo: properly check for overruns" Jan Beulich
2014-11-03 15:02 ` [PATCH 2/2] lzo: check for length overrun in variable length encoding Jan Beulich
2014-11-04 11:24 ` Ian Jackson
2014-11-04 11:35 ` Ian Campbell
2014-11-04 9:37 ` [PATCH 0/2] lzo: refine overrun checking Ian Campbell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).