Valgrind fixes

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

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.

---
 libdm/ioctl/libdm-iface.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

index 2f96a0f..b99f885 100644
@@ -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

Needing more context for this one:
Under what conditions is the bug triggered?
What are the implications when it is?

Alasdair

Dne 4.3.2011 21:39, Alasdair G Kergon napsal(a):

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

Ack.

Alasdair

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...

---
 daemons/clvmd/clvmd.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

index 00d330d..ecc366d 100644
@@ -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

Dne 2.3.2011 19:09, Zdenek Kabelac napsal(a):

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>

What are the implications beyond the immediate line of code changed?

What are the implications of not setting these to 0?

Alasdair

Dne 4.3.2011 21:42, Alasdair G Kergon napsal(a):

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.

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

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.

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