Re: [PATCH 4/4] device/chario.c (char_write): avoid segmentation fault
On Sun, Dec 15, 2013 at 02:43:50PM +0100, Marin Ramesa wrote: > On 15.12.2013 14:00:22, Richard Braun wrote: > > What makes you think the content could be something else than a > > vm_map_copy object ? > > Well io_data is a pointer to char, not a pointer to vm_map_copy. And > there is not one member in io_req structure that keeps track of the > io_data size. The function char_write() could be called with io_data > having it's origin in something other than vm_map_copy. You're reasoning only on what you see at the language level. If you look more closely, you'll see this case only applies when the data hasn't been transferred in-band. I'm pretty certain the VM system will have used a vm_map_copy object to list the pages concerned in the ou-of-band case. > > No, nothing guarantees that structures are null-terminated. This is a > > very wrong assumption. In addition, even if it was possible for the > > data to be something else than a vm_map_copy (in which case we'd want > > an assertion, because it should *never* happen), the data size could > > be 0, in which case simply accessing the first byte might cause a > > crash. > > The tests I've run always show null-termination. But you're right, the > structure could very well contain a '\0' in which case strlen() > wouldn't work. But there has to be some way to detect the end of a > structure in memory without knowing the types. Now you're reasoning only on what you saw with a few tests ! And no, what you want to do here makes no sense. You can't "detect" the end of a structure in memory. You have to know, either directly, or through some mechanism (e.G. a header containing the real type and length). -- Richard Braun
Re: [PATCH 4/4] device/chario.c (char_write): avoid segmentation fault
On 15.12.2013 14:00:22, Richard Braun wrote: > On Sat, Dec 14, 2013 at 01:32:27PM +0100, Marin Ramesa wrote: > > On 14.12.2013 12:45:32, Richard Braun wrote: > > > I really don't see a problem in that code, you'll have to > > > describe it better. > > > > So a pointer to io_data is cast to a pointer to a vm_map_copy > > structure and then passed to vm_map_copyout() which uses the > > vm_map_copy structure members as if the contents of the memory at > > the source address were vm_map_copy structure objects, not objects > > of io_data which is of unknown size and origin to the function. > > This can lead to a segmentation fault, as is shown in that short > > test program. > > > > My patch makes sure the io_data actually came from a vm_map_copy > > structure. > > What makes you think the content could be something else than a > vm_map_copy object ? Well io_data is a pointer to char, not a pointer to vm_map_copy. And there is not one member in io_req structure that keeps track of the io_data size. The function char_write() could be called with io_data having it's origin in something other than vm_map_copy. > > > On the other hand, your patch relies on knowing the target > > > types, although vm_map_copy_t is explicitely described as being > > > opaque. > > > > I don't see a problem with it. There is an explicit cast to > > vm_map_copy_t. It's first member type is known. > > The target type of the cast is a pointer (a very bad historical > practice in Mach that consists of typedef'ing pointers to structures > in order to somehow add an abstraction level intended to make the > coding style more object-oriented). In proper C, a cast means the > programmer is certain about the data type. Here, it is used to call > vm_map_copyout without emitting warnings. It has nothing to do with > accessing the data itself, just passing it around, so no, the member > type is not know, not from the point of view of modules outside the > VM system. By doing that, you're violating the intended opacity of > the type (see vm_map.h, "vm_map_copy_t [exported; contents > invisible]"). The problem with that approach is that, if someone, > someday, decides to change that type, he wouldn't know he would also > have to change the code you introduce, and actually, he shouldn't > have to. That knowledge should be completely contained in > the vm_map module. OK, I get it. > > > and I also don't understand why you assume the data to be null- > > > terminated. > > > > Structures are always null-terminated in memory. So if data came > > from a structure (like vm_map_copy) it has to be null-terminated. > > If not, a random '\0' will be found, sizes will not match, and > > function will return. > > No, nothing guarantees that structures are null-terminated. This is a > very wrong assumption. In addition, even if it was possible for the > data to be something else than a vm_map_copy (in which case we'd want > an assertion, because it should *never* happen), the data size could > be 0, in which case simply accessing the first byte might cause a > crash. The tests I've run always show null-termination. But you're right, the structure could very well contain a '\0' in which case strlen() wouldn't work. But there has to be some way to detect the end of a structure in memory without knowing the types.
Re: [PATCH 4/4] device/chario.c (char_write): avoid segmentation fault
On Sat, Dec 14, 2013 at 01:32:27PM +0100, Marin Ramesa wrote: > On 14.12.2013 12:45:32, Richard Braun wrote: > > I really don't see a problem in that code, you'll have to describe it > > better. > > So a pointer to io_data is cast to a pointer to a vm_map_copy structure > and then passed to vm_map_copyout() which uses the vm_map_copy > structure members as if the contents of the memory at the source > address were vm_map_copy structure objects, not objects of io_data > which is of unknown size and origin to the function. This can lead to a > segmentation fault, as is shown in that short test program. > > My patch makes sure the io_data actually came from a vm_map_copy > structure. What makes you think the content could be something else than a vm_map_copy object ? > > On the other hand, your patch relies on knowing the target > > types, although vm_map_copy_t is explicitely described as being > > opaque. > > I don't see a problem with it. There is an explicit cast to > vm_map_copy_t. It's first member type is known. The target type of the cast is a pointer (a very bad historical practice in Mach that consists of typedef'ing pointers to structures in order to somehow add an abstraction level intended to make the coding style more object-oriented). In proper C, a cast means the programmer is certain about the data type. Here, it is used to call vm_map_copyout without emitting warnings. It has nothing to do with accessing the data itself, just passing it around, so no, the member type is not know, not from the point of view of modules outside the VM system. By doing that, you're violating the intended opacity of the type (see vm_map.h, "vm_map_copy_t [exported; contents invisible]"). The problem with that approach is that, if someone, someday, decides to change that type, he wouldn't know he would also have to change the code you introduce, and actually, he shouldn't have to. That knowledge should be completely contained in the vm_map module. > > and I also don't understand why you assume the data to be null- > > terminated. > > Structures are always null-terminated in memory. So if data came from a > structure (like vm_map_copy) it has to be null-terminated. If not, a > random '\0' will be found, sizes will not match, and function will > return. No, nothing guarantees that structures are null-terminated. This is a very wrong assumption. In addition, even if it was possible for the data to be something else than a vm_map_copy (in which case we'd want an assertion, because it should *never* happen), the data size could be 0, in which case simply accessing the first byte might cause a crash. -- Richard Braun
Re: [PATCH 4/4] device/chario.c (char_write): avoid segmentation fault
On 14.12.2013 12:45:32, Richard Braun wrote: > On Fri, Dec 13, 2013 at 09:06:55PM +0100, Marin Ramesa wrote: > > Check if the source of the data in the source structure > > memory is a target structure data. Do this by comparing the > > length of the char values starting with the source address until > > null-termination to the size of the target structure. Start by > > initializing the counter to the size of the first member in the > > target structure. Then char values in memory are counted until > > null-termination, and finally the counted result is compared to the > > size of the target structure. If the values don't match, return > > from the function. > > I really don't see a problem in that code, you'll have to describe it > better. So a pointer to io_data is cast to a pointer to a vm_map_copy structure and then passed to vm_map_copyout() which uses the vm_map_copy structure members as if the contents of the memory at the source address were vm_map_copy structure objects, not objects of io_data which is of unknown size and origin to the function. This can lead to a segmentation fault, as is shown in that short test program. My patch makes sure the io_data actually came from a vm_map_copy structure. > On the other hand, your patch relies on knowing the target > types, although vm_map_copy_t is explicitely described as being > opaque. I don't see a problem with it. There is an explicit cast to vm_map_copy_t. It's first member type is known. But maybe I don't understand what it means for a type to be opaque. > In addition, you basically reimplement strlen inline, OK then, I will take a look at strlen. > and I also don't understand why you assume the data to be null- > terminated. Structures are always null-terminated in memory. So if data came from a structure (like vm_map_copy) it has to be null-terminated. If not, a random '\0' will be found, sizes will not match, and function will return.
Re: [PATCH 4/4] device/chario.c (char_write): avoid segmentation fault
On Fri, Dec 13, 2013 at 09:06:55PM +0100, Marin Ramesa wrote: > Check if the source of the data in the source structure > memory is a target structure data. Do this by comparing the > length of the char values starting with the source address until > null-termination to the size of the target structure. Start by > initializing the counter to the size of the first member in the > target structure. Then char values in memory are counted until > null-termination, and finally the counted result is compared to the > size of the target structure. If the values don't match, return from the > function. I really don't see a problem in that code, you'll have to describe it better. On the other hand, your patch relies on knowing the target types, although vm_map_copy_t is explicitely described as being opaque. In addition, you basically reimplement strlen inline, and I also don't understand why you assume the data to be null-terminated. -- Richard Braun
[PATCH 4/4] device/chario.c (char_write): avoid segmentation fault
The situation is that there is a pointer (source) that is being cast to
another pointer (target) and then the members of the target structure
pointed to by the target pointer are being used as if the content of
memory at the source address is a target structure, not source structure
pointed to by the source pointer.
In the worst case (when source structure is smaller than the target
structure) this leads to segmentation fault, as is illustrated by
the following short program:
struct s1 {
int x1;
int x2;
};
struct s2 {
int x3;
};
int main()
{
int x = 0;
int y = 1;
struct s1 *y1 = (struct s1 *)&x;
struct s2 *y2 = (struct s2 *)&y;
y1->x1 = 0;
y1->x2 = 0;
y2->x3 = 1;
((struct s1 *)y2)->x2;
return 0;
}
In the better case (when target structure is smaller or equal to the
source structure and the source of the data in the source structure
is not the target structure) this leads to usage of incorrect values.
Check if the source of the data in the source structure
memory is a target structure data. Do this by comparing the
length of the char values starting with the source address until
null-termination to the size of the target structure. Start by
initializing the counter to the size of the first member in the
target structure. Then char values in memory are counted until
null-termination, and finally the counted result is compared to the
size of the target structure. If the values don't match, return from the
function.
* device/chario.c (char_write) (i, temp): New variables.
(char_write): New do-while loop.
(char_write): New if statement.
---
device/chario.c | 8
1 file changed, 8 insertions(+)
diff --git a/device/chario.c b/device/chario.c
index 91bd8e8..d962c5c 100644
--- a/device/chario.c
+++ b/device/chario.c
@@ -268,6 +268,14 @@ io_return_t char_write(
vm_map_copy_t copy = (vm_map_copy_t) data;
kern_return_t kr;
+ int i = sizeof(int);
+ char *temp = data;
+
+ do i++; while(*temp++ != '\0');
+
+ if (i != sizeof(struct vm_map_copy))
+ return KERN_INVALID_ARGUMENT;
+
kr = vm_map_copyout(device_io_map, &addr, copy);
if (kr != KERN_SUCCESS)
return kr;
--
1.8.1.4
