Bug 1455263 - invalid memory read in copy_line, called by define_commands in command.c
Summary: invalid memory read in copy_line, called by define_commands in command.c
Keywords:
Status: NEW
Alias: None
Product: LVM and device-mapper
Classification: Community
Component: lvm2
Version: 2.02.171
Hardware: Unspecified
OS: Unspecified
low
unspecified
Target Milestone: ---
: ---
Assignee: David Teigland
QA Contact: cluster-qe
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-05-24 15:09 UTC by hanno
Modified: 2024-06-13 13:20 UTC (History)
6 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed:
Embargoed:
rule-engine: lvm-technical-solution?
rule-engine: lvm-test-coverage?


Attachments (Terms of Use)
patch / fix (324 bytes, patch)
2017-05-24 15:09 UTC, hanno
no flags Details | Diff
address sanitizer error / stack trace (6.32 KB, text/plain)
2017-05-24 16:03 UTC, hanno
no flags Details

Description hanno 2017-05-24 15:09:39 UTC
Created attachment 1281999 [details]
patch / fix

Description of problem:

Testing the lvm command line tool with address sanitizer uncovers an invalid memory read happening in the function copy_line.c


Version-Release number of selected component (if applicable):

LVM2.2.02.171


How reproducible:
always

Steps to Reproduce:
1. compile LVM2 with address sanitizer
2. run tools/lvm
3. see error

Actual results:

ASAN reports global buffer overflow


Expected results:

Tool starts normally.


Additional info:

This is the affected code in the function copy_line():

	while (1) {
		line[i] = _command_input[p];
		i++;
		p++;

		if (_command_input[p] == '\n') {
			p++;
			break;
		}

		if (i == (max_line - 1))
			break;
	}

This will read until a newline and then exit the loop. However the calling function define_commands() assumes a trailing newline in the returned string to mark the end of the string. It will also use a check of the form 		if (line[0] == '\n')
to check whether we're at the end of parsing the command string. This obviously only works if the newline byte is also copied by copy_line, which it isn't.

A possible solution is to switch the increasing of the i and p variables and the check for the newline byte in copy_line, thus making sure that it's only checked after the newline character is already copied:
	while (1) {
		line[i] = _command_input[p];

		if (_command_input[p] == '\n') {
			p++;
			break;
		}
		i++;
		p++;

		if (i == (max_line - 1))
			break;
	}

I'll attach a patch to do this.

Comment 1 hanno 2017-05-24 16:03:34 UTC
Created attachment 1282002 [details]
address sanitizer error / stack trace

Comment 2 David Teigland 2017-05-24 17:20:17 UTC
thanks, will put this in soon, there seems to be no actual problem since \n isn't actually needed in practice.

Comment 3 hanno 2018-02-04 17:41:30 UTC
There's recently been a new version of lvm2, yet this has been ignored since I reported it. Anything I can do so you'll actually review this and commit it?

It's a pretty straightforward bug and fix.

Comment 4 David Teigland 2018-02-05 16:06:36 UTC
The patch appears to cause a segfault

/bin/sh: line 1: 15696 Segmentation fault      ../tools/man-generator --primary lvmconfig > test.gen

Comment 7 Henrik Holst 2024-06-13 13:20:28 UTC
Is this the same error?

```
# lvm
lvm> version
  LVM version:     2.03.24(2) (2024-05-16)
  Library version: 1.02.198 (2024-05-16)
  Driver version:  4.48.0
  Configuration:   ./configure CONFIG_SHELL=/bin/bash --prefix=/usr --sbindir=/usr/bin --sysconfdir=/etc --localstatedir=/var --enable-cmdlib --enable-dmeventd --enable-lvmpolld --enable-pkgconfig --enable-readline --enable-udev_rules --enable-udev_sync --enable-write_install --with-cache=internal --with-default-dm-run-dir=/run --with-default-locking-dir=/run/lock/lvm --with-default-pid-dir=/run --with-default-run-dir=/run/lvm --with-libexecdir=/usr/lib/lvm2 --with-systemdsystemunitdir=no --with-thin=internal --with-udev-prefix=/usr
lvm> help foo
  foo - (null)

fish: Job 1, 'lvm' terminated by signal SIGSEGV (Address boundary error)
```


Note You need to log in before you can comment on or make changes to this bug.