Re: [Qemu-devel] [PATCH v3 5/8] block: Fix potential Null pointer dereferences in vvfat.c
On 12/10/18 16:14, Max Reitz wrote:
On 31.08.18 20:16, Liam Merwick wrote:
The calls to bdrv_new_open_driver(), find_mapping_for_cluster(),
and array_get_next() may return NULL but it isn't always checked for
before dereferencing the value returned.
Signed-off-by: Liam Merwick
Reviewed-by: Darren Kenny
Reviewed-by: Mark Kanda
---
block/vvfat.c | 56
1 file changed, 56 insertions(+)
diff --git a/block/vvfat.c b/block/vvfat.c
index fc41841a5c3c..0f1f10a2f94b 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -448,6 +448,9 @@ static direntry_t *create_long_filename(BDRVVVFATState *s,
const char *filename)
for(i=0;i
entry=array_get_next(&(s->directory));
+if (entry == NULL) {
+continue;
+}
This is a bug in array_ensure_allocated(). It uses g_realloc() with a
non-zero size, so that function will never return NULL. It will rather
abort().
Therefore, array_ensure_allocated() cannot fail. Consequentially,
array_get_next() cannot fail.
I've reverted this (and the rest of the 'As above' comments below)
entry->attributes=0xf;
entry->reserved[0]=0;
entry->begin=0;
@@ -665,6 +668,9 @@ static inline void fat_set(BDRVVVFATState* s,unsigned int
cluster,uint32_t value
} else {
int offset = (cluster*3/2);
unsigned char* p = array_get(&(s->fat), offset);
+if (p == NULL) {
+return;
+}
This is only reached if array_get_next() was called before. Therefore,
this cannot return NULL.
However, an assert(array->pointer); in array_get() can't hurt.
Done.
switch (cluster&1) {
case 0:
p[0] = value&0xff;
@@ -730,6 +736,9 @@ static inline direntry_t*
create_short_and_long_name(BDRVVVFATState* s,
if(is_dot) {
entry=array_get_next(&(s->directory));
+if (entry == NULL) {
+return NULL;
+}
As above.
memset(entry->name, 0x20, sizeof(entry->name));
memcpy(entry->name,filename,strlen(filename));
return entry;
@@ -844,6 +853,12 @@ static int read_directory(BDRVVVFATState* s, int
mapping_index)
/* create mapping for this file */
if(!is_dot && !is_dotdot && (S_ISDIR(st.st_mode) || st.st_size)) {
s->current_mapping = array_get_next(&(s->mapping));
+if (s->current_mapping == NULL) {
+fprintf(stderr, "Failed to create mapping for file\n");
+g_free(buffer);
+closedir(dir);
+return -2;
+}
As above.
s->current_mapping->begin=0;
s->current_mapping->end=st.st_size;
/*
@@ -941,6 +956,9 @@ static int init_directories(BDRVVVFATState* s,
/* add volume label */
{
direntry_t* entry=array_get_next(&(s->directory));
+if (entry == NULL) {
+return -1;
+}
As above.
entry->attributes=0x28; /* archive | volume label */
memcpy(entry->name, s->volume_label, sizeof(entry->name));
}
@@ -953,6 +971,9 @@ static int init_directories(BDRVVVFATState* s,
s->cluster_count=sector2cluster(s, s->sector_count);
mapping = array_get_next(&(s->mapping));
+if (mapping == NULL) {
+return -1;
+}
As above.
mapping->begin = 0;
mapping->dir_index = 0;
mapping->info.dir.parent_mapping_index = -1;
@@ -1630,6 +1651,9 @@ static void schedule_rename(BDRVVVFATState* s,
uint32_t cluster, char* new_path)
{
commit_t* commit = array_get_next(&(s->commits));
+if (commit == NULL) {
+return;
+}
As above.
commit->path = new_path;
commit->param.rename.cluster = cluster;
commit->action = ACTION_RENAME;
@@ -1639,6 +1663,9 @@ static void schedule_writeout(BDRVVVFATState* s,
int dir_index, uint32_t modified_offset)
{
commit_t* commit = array_get_next(&(s->commits));
+if (commit == NULL) {
+return;
+}
As above.
commit->path = NULL;
commit->param.writeout.dir_index = dir_index;
commit->param.writeout.modified_offset = modified_offset;
@@ -1649,6 +1676,9 @@ static void schedule_new_file(BDRVVVFATState* s,
char* path, uint32_t first_cluster)
{
commit_t* commit = array_get_next(&(s->commits));
+if (commit == NULL) {
+return;
+}
As above.
commit->path = path;
commit->param.new_file.first_cluster = first_cluster;
commit->action = ACTION_NEW_FILE;
@@ -1657,6 +1687,9 @@ static void schedule_new_file(BDRVVVFATState* s,
static void schedule_mkdir(BDRVVVFATState* s, uint32_t cluster, char* path)
{
commit_t* commit = array_get_next(&(s->commits));
+if (commit == NULL) {
+return;
+}
As above.
commit->path = path;
commit->param.mkdir.cluster = cluster;
commit->action =
Re: [Qemu-devel] [PATCH v3 5/8] block: Fix potential Null pointer dereferences in vvfat.c
On 31.08.18 20:16, Liam Merwick wrote:
> The calls to bdrv_new_open_driver(), find_mapping_for_cluster(),
> and array_get_next() may return NULL but it isn't always checked for
> before dereferencing the value returned.
>
> Signed-off-by: Liam Merwick
> Reviewed-by: Darren Kenny
> Reviewed-by: Mark Kanda
> ---
> block/vvfat.c | 56
> 1 file changed, 56 insertions(+)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index fc41841a5c3c..0f1f10a2f94b 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -448,6 +448,9 @@ static direntry_t *create_long_filename(BDRVVVFATState
> *s, const char *filename)
>
> for(i=0;i entry=array_get_next(&(s->directory));
> +if (entry == NULL) {
> +continue;
> +}
This is a bug in array_ensure_allocated(). It uses g_realloc() with a
non-zero size, so that function will never return NULL. It will rather
abort().
Therefore, array_ensure_allocated() cannot fail. Consequentially,
array_get_next() cannot fail.
> entry->attributes=0xf;
> entry->reserved[0]=0;
> entry->begin=0;
> @@ -665,6 +668,9 @@ static inline void fat_set(BDRVVVFATState* s,unsigned int
> cluster,uint32_t value
> } else {
> int offset = (cluster*3/2);
> unsigned char* p = array_get(&(s->fat), offset);
> +if (p == NULL) {
> +return;
> +}
This is only reached if array_get_next() was called before. Therefore,
this cannot return NULL.
However, an assert(array->pointer); in array_get() can't hurt.
> switch (cluster&1) {
> case 0:
> p[0] = value&0xff;
> @@ -730,6 +736,9 @@ static inline direntry_t*
> create_short_and_long_name(BDRVVVFATState* s,
>
> if(is_dot) {
> entry=array_get_next(&(s->directory));
> +if (entry == NULL) {
> +return NULL;
> +}
As above.
> memset(entry->name, 0x20, sizeof(entry->name));
> memcpy(entry->name,filename,strlen(filename));
> return entry;
> @@ -844,6 +853,12 @@ static int read_directory(BDRVVVFATState* s, int
> mapping_index)
> /* create mapping for this file */
> if(!is_dot && !is_dotdot && (S_ISDIR(st.st_mode) || st.st_size)) {
> s->current_mapping = array_get_next(&(s->mapping));
> +if (s->current_mapping == NULL) {
> +fprintf(stderr, "Failed to create mapping for file\n");
> +g_free(buffer);
> +closedir(dir);
> +return -2;
> +}
As above.
> s->current_mapping->begin=0;
> s->current_mapping->end=st.st_size;
> /*
> @@ -941,6 +956,9 @@ static int init_directories(BDRVVVFATState* s,
> /* add volume label */
> {
> direntry_t* entry=array_get_next(&(s->directory));
> +if (entry == NULL) {
> +return -1;
> +}
As above.
> entry->attributes=0x28; /* archive | volume label */
> memcpy(entry->name, s->volume_label, sizeof(entry->name));
> }
> @@ -953,6 +971,9 @@ static int init_directories(BDRVVVFATState* s,
> s->cluster_count=sector2cluster(s, s->sector_count);
>
> mapping = array_get_next(&(s->mapping));
> +if (mapping == NULL) {
> +return -1;
> +}
As above.
> mapping->begin = 0;
> mapping->dir_index = 0;
> mapping->info.dir.parent_mapping_index = -1;
> @@ -1630,6 +1651,9 @@ static void schedule_rename(BDRVVVFATState* s,
> uint32_t cluster, char* new_path)
> {
> commit_t* commit = array_get_next(&(s->commits));
> +if (commit == NULL) {
> +return;
> +}
As above.
> commit->path = new_path;
> commit->param.rename.cluster = cluster;
> commit->action = ACTION_RENAME;
> @@ -1639,6 +1663,9 @@ static void schedule_writeout(BDRVVVFATState* s,
> int dir_index, uint32_t modified_offset)
> {
> commit_t* commit = array_get_next(&(s->commits));
> +if (commit == NULL) {
> +return;
> +}
As above.
> commit->path = NULL;
> commit->param.writeout.dir_index = dir_index;
> commit->param.writeout.modified_offset = modified_offset;
> @@ -1649,6 +1676,9 @@ static void schedule_new_file(BDRVVVFATState* s,
> char* path, uint32_t first_cluster)
> {
> commit_t* commit = array_get_next(&(s->commits));
> +if (commit == NULL) {
> +return;
> +}
As above.
> commit->path = path;
> commit->param.new_file.first_cluster = first_cluster;
> commit->action = ACTION_NEW_FILE;
> @@ -1657,6 +1687,9 @@ static void schedule_new_file(BDRVVVFATState* s,
> static void schedule_mkdir(BDRVVVFATState* s, uint32_t cluster, char* path)
> {
> commit_t* commit = array_get_next(&(s->commits));
> +if (commit == NULL) {
> +return;
> +}
As above.
> commit->path = path;
> commit->param.mkdir.cluster =
