[PATCH 0/2] Valgrind fixes

[PATCH 0/2] Valgrind fixes

[Zdenek Kabelac]

While hunting for sharing vg bugs - few unrelated problems appeared.
Here is the first set that tries to address reported problems.

Zdenek Kabelac (2):
  Fix read byte from char[-1] position
  Do not send random bytes in message

 daemons/clvmd/clvmd.c     |    9 +++++++--
 libdm/ioctl/libdm-iface.c |    6 ++++--
 2 files changed, 11 insertions(+), 4 deletions(-)

-- 
1.7.4.1

[PATCH 1/2] Fix read byte from char[-1] position

[Zdenek Kabelac]

When the ->params is empty string - access is made on the byte
before allocated buffer (catched by valgrind) - in case it would
constains 0x20 - it would even overwrite this buffer.
So fix by checking len > 0 before doing such access.

Optimise the code and use len counter instead of multiple strlen calls.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 libdm/ioctl/libdm-iface.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/libdm/ioctl/libdm-iface.c b/libdm/ioctl/libdm-iface.c
index 2f96a0f..b99f885 100644
--- a/libdm/ioctl/libdm-iface.c
+++ b/libdm/ioctl/libdm-iface.c
@@ -1836,6 +1836,7 @@ static int _reload_with_suppression_v4(struct dm_task *dmt)
 {
 	struct dm_task *task;
 	struct target *t1, *t2;
+	size_t len;
 	int r;
 
 	/* New task to get existing table information */
@@ -1878,8 +1879,9 @@ static int _reload_with_suppression_v4(struct dm_task *dmt)
 	t2 = task->head;
 
 	while (t1 && t2) {
-		while (t2->params[strlen(t2->params) - 1] == ' ')
-			t2->params[strlen(t2->params) - 1] = '\0';
+		len = strlen(t2->params);
+		while (len-- > 0 && t2->params[len] == ' ')
+			t2->params[len] = '\0';
 		if ((t1->start != t2->start) ||
 		    (t1->length != t2->length) ||
 		    (strcmp(t1->type, t2->type)) ||
-- 
1.7.4.1

[PATCH 1/2] Fix read byte from char[-1] position

[Alasdair G Kergon]

On Wed, Mar 02, 2011 at 07:09:07PM +0100, Zdenek Kabelac wrote:
> When the ->params is empty string - access is made on the byte
> before allocated buffer (catched by valgrind) - in case it would
> constains 0x20 - it would even overwrite this buffer.
> So fix by checking len > 0 before doing such access.
> Optimise the code and use len counter instead of multiple strlen calls.
 
Needing more context for this one:
  Under what conditions is the bug triggered?
  What are the implications when it is?

Alasdair

[PATCH 1/2] Fix read byte from char[-1] position

[Zdenek Kabelac]

Dne 4.3.2011 21:39, Alasdair G Kergon napsal(a):
> On Wed, Mar 02, 2011 at 07:09:07PM +0100, Zdenek Kabelac wrote:
>> When the ->params is empty string - access is made on the byte
>> before allocated buffer (catched by valgrind) - in case it would
>> constains 0x20 - it would even overwrite this buffer.
>> So fix by checking len > 0 before doing such access.
>> Optimise the code and use len counter instead of multiple strlen calls.
>  
> Needing more context for this one:
>   Under what conditions is the bug triggered?
>   What are the implications when it is?
> 

On my box this happend when 'error' target without params is activated for
fixing mirror
(t-activate-partial.sh   -  'lvchange -v --refresh --partial $vg/mirror')
Fixed table looks like this:

@PREFIX at vg-mirror_mimage_0: 0 8192 linear 253:13 0
mvg-lvol3: 0 256 linear 7:0 2816
mvg-mir_mimage_2: 0 2560 linear 7:0 8192
mvg-lvol2: 0 256 linear 7:0 2560
mvg-mir_mimage_1: 0 2560 linear 7:0 5632
mvg-lvol1: 0 256 linear 7:0 2304
@PREFIX at vg-mirror_mlog: 0 8192 linear 253:11 2048
mvg-mir_mimage_0: 0 2560 linear 7:0 3072
@PREFIX at vg-mirror: 0 8192 mirror disk 2 253:12 1024 2 253:14 0 253:15 0 1
handle_errors
mvg-lvol0: 0 256 linear 7:0 2048
@PREFIX at pv3: 0 68949 linear 7:1 137898
mvg-mir: 0 2560 mirror disk 2 253:4 512 3 253:5 0 253:6 0 253:7 0 1 handle_errors
@PREFIX at pv2: 0 68949 linear 7:1 68949
@PREFIX at pv1: 0 10000000 error
@PREFIX at vg-mirror_mimage_0-missing_0_0: 0 8192 error
mvg-mir_mlog: 0 256 linear 7:0 10752
@PREFIX at vg-mirror_mimage_1: 0 8192 linear 253:10 2048


On my little endian machine - this byte seems to be usually 0x00 - thus on
x86_64 it will just make a test for 0x20 and skip the loop - no write happens.

It's quite hard to guess what is actually stored before the allocated params
string starts - probably some internal malloc data - if I'd take is as 8byte
value - there is  0x21 in little endian form - which could mean that on big
endian machine  0x21 could be located on the checked address - so it's hard to
estimate whether there could be actually 0x20.

In case there would 0x20 - it would be overwritten with 0 - next loop check
would exit (0 != 0x20) - but the memory is already modified and it would
probably mean memory corruption (this is heavily dependent on how the malloc
is implemented).

So my guess is - on LE it's mostly harmless just access outside of allocated
memory, on BE it might eventually trigger some hard to estimate problems.

Zdenek

[PATCH 1/2] Fix read byte from char[-1] position

[Alasdair G Kergon]

On Wed, Mar 02, 2011 at 07:09:07PM +0100, Zdenek Kabelac wrote:
> When the ->params is empty string - access is made on the byte
> before allocated buffer (catched by valgrind) - in case it would
> constains 0x20 - it would even overwrite this buffer.
> So fix by checking len > 0 before doing such access.
 
Ack.

Alasdair

[PATCH 2/2] Do not send random bytes in message

[Zdenek Kabelac]

Fixing few issues:

struct clvm_header contains 'char args[1]' - so adding '+ 1' here
for the message length calculation is not correct - we end up with longer
message where last byte is uninitialized and passed to write function.

xid and clintid are initialized to 0.

Memory allocation is checked for NULL - though it's not really clear what
should happen in this case - so just log the message - probably it will 
fail few moments later...

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 daemons/clvmd/clvmd.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/daemons/clvmd/clvmd.c b/daemons/clvmd/clvmd.c
index 00d330d..ecc366d 100644
--- a/daemons/clvmd/clvmd.c
+++ b/daemons/clvmd/clvmd.c
@@ -1743,13 +1743,18 @@ static void send_local_reply(struct local_client *client, int status, int fd)
 	}
 
 	/* Add in the size of our header */
-	message_len = message_len + sizeof(struct clvm_header) + 1;
-	replybuf = malloc(message_len);
+	message_len = message_len + sizeof(struct clvm_header);
+	if (!(replybuf = malloc(message_len))) {
+		DEBUGLOG("Memory allocation fails\n");
+		return;
+	}
 
 	clientreply = (struct clvm_header *) replybuf;
 	clientreply->status = status;
 	clientreply->cmd = CLVMD_CMD_REPLY;
 	clientreply->node[0] = '\0';
+	clientreply->xid = 0;
+	clientreply->clientid = 0;
 	clientreply->flags = 0;
 
 	ptr = clientreply->args;
-- 
1.7.4.1

[PATCH 2/2] Do not send random bytes in message

[Zdenek Kabelac]

Dne 2.3.2011 19:09, Zdenek Kabelac napsal(a):
> Fixing few issues:
> 
> struct clvm_header contains 'char args[1]' - so adding '+ 1' here
> for the message length calculation is not correct - we end up with longer
> message where last byte is uninitialized and passed to write function.
> 
> xid and clintid are initialized to 0.
> 
> Memory allocation is checked for NULL - though it's not really clear what
> should happen in this case - so just log the message - probably it will 
> fail few moments later...
> 

Needs also updated arglen parameter - so attaching updated patch.

Zdenek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0000-Do-not-send-random-bytes-in-message.patch
Type: text/x-patch
Size: 1722 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20110302/e1f26eb4/attachment.bin>

[PATCH 2/2] Do not send random bytes in message

[Alasdair G Kergon]

On Wed, Mar 02, 2011 at 07:09:08PM +0100, Zdenek Kabelac wrote:
> struct clvm_header contains 'char args[1]' - so adding '+ 1' here
> for the message length calculation is not correct - we end up with longer
> message where last byte is uninitialized and passed to write function.
 
What are the implications beyond the immediate line of code changed?

> xid and clintid are initialized to 0.
 
What are the implications of not setting these to 0?

Alasdair

[PATCH 2/2] Do not send random bytes in message

[Zdenek Kabelac]

Dne 4.3.2011 21:42, Alasdair G Kergon napsal(a):
> On Wed, Mar 02, 2011 at 07:09:08PM +0100, Zdenek Kabelac wrote:
>> struct clvm_header contains 'char args[1]' - so adding '+ 1' here
>> for the message length calculation is not correct - we end up with longer
>> message where last byte is uninitialized and passed to write function.
>  
> What are the implications beyond the immediate line of code changed?

All messages will have consistent form and will no contain random
(uninitialized) data in their body.

It may mean it will 'leak' some bytes from reallocated memory pool, however we
do not release any sensitive data and leak bytes are very short sequences,
there is IMHO no security problem.


> 
>> xid and clintid are initialized to 0.
>  
> What are the implications of not setting these to 0?

Currently I'd say that only sending of random data in messages and producing
valgrind warnings with each code run.

Of course, it's probably more wide spread through the clvmd code than my
current patch set fixed - I'm planning to add few more later...

As 'char args[1]' in clvmd_header is defined as taking 1 byte (common tactic
is probably to use there 'char args[0]' - but this change would require much
bigger rewrite) - data are written already at position [0] - not after the
sizeof(clvm_header) so it's commonly used this way:
   - message_len =  sizeof(clvm_header) + strlen(added_chunks) + 1.
this way the last byte is actually not set and left in a form which came from
memory allocation. As the size of the message is deduced from 'arglen'
variable - stripping the message from the last random value byte should not
cause problems.

Another problem in this code seems to be that some parts of clvm_header are
left undefined (using values from allocated memory chunk - usually xid) - I
hope it's not part of protocol desing to use such randomness inside the
message - from my code check it's probably only issue for local messages where
this values are ignored.

Zdenek

[PATCH 2/2] Do not send random bytes in message

[Alasdair G Kergon]

On Wed, Mar 02, 2011 at 07:09:08PM +0100, Zdenek Kabelac wrote:
> struct clvm_header contains 'char args[1]' - so adding '+ 1' here
> for the message length calculation is not correct - we end up with longer
> message where last byte is uninitialized and passed to write function.
 
That size calcluation is unclear to me: I doubt that args[1] was the reason for
the +1.  As long as you have confirmed there are no types/combinations of
messages or architectures that need it, ack.

> xid and clintid are initialized to 0.
> 
> Memory allocation is checked for NULL - though it's not really clear what
> should happen in this case - so just log the message - probably it will 
> fail few moments later...
 
I think it's important here that malloc cannot fail:)
(Does it inherit the right lock-into-memory settings from its lvm linking or
is something missing?)

Ack.

Alasdair