hfs: fix hfs_readdir()

PATCH Superseded Dan Carpenter 2 participants 8 messages Jan 26, 2016 – Jan 18, 2017

Subsystems (3)

VFS Kernel janitors HFS filesystem

lore

Share & tools

lore

Dan Carpenter

author

2016-01-26 09:26 UTC #0 lore raw

PATCH 1/1 hfs: fix hfs_readdir()

I was looking through static analysis warnings and we seem to be copying garbage into &rd->key. This goes back to before the start of git...

Signed-off-by Dan Carpenter <[email protected]>

---
Not tested. Please review carefully.

fs/hfs/dir.c +1 -1

index 70788e0..66485d7 100644
@@ -163,7 +163,7 @@ static int hfs_readdir(struct file *file, struct dir_context *ctx)
 		rd->file = file;
 		list_add(&rd->list, &HFS_I(inode)->open_dir_list);
 	}
-	memcpy(&rd->key, &fd.key, sizeof(struct hfs_cat_key));
+	memcpy(&rd->key, &fd.key->cat, sizeof(struct hfs_cat_key));
 out:
 	hfs_find_exit(&fd);
 	return err;

Viacheslav Dubeyko

maintainer

2016-01-26 18:18 UTC #1 lore raw

Quoted 12 lines · Dan Carpenter

I was looking through static analysis warnings and we seem to be copying garbage into &rd->key. This goes back to before the start of git...
Signed-off-by Dan Carpenter <[email protected]>
---
Not tested. Please review carefully.
fs/hfs/dir.c +0 -0
index 70788e0..66485d7 100644

fs/hfs/dir.c static int hfs_readdir(struct file *file, struct dir_context *ctx) · line 166

Quoted 5 lines

fs/hfs/dir.c +1 -1

@@ -163,7 +163,7 @@ static int hfs_readdir(struct file *file, struct dir_context *ctx)
 		rd->file = file;
 		list_add(&rd->list, &HFS_I(inode)->open_dir_list);
 	}
-	memcpy(&rd->key, &fd.key, sizeof(struct hfs_cat_key));
+	memcpy(&rd->key, &fd.key->cat, sizeof(struct hfs_cat_key));

The field "key" is union:

164 typedef union hfs_btree_key {

165         u8 key_len;                     /* number of bytes in the key */
166         struct hfs_cat_key cat;
167         struct hfs_ext_key ext;

168 } hfs_btree_key;

The struct hfs_cat_key is the biggest item. So, size of this structure is dominating in the union:

157 struct hfs_ext_key {

158         u8 key_len;             /* number of bytes in the key */
159         u8 FkType;              /* HFS_FK_{DATA,RSRC} */
160         __be32 FNum;            /* The File ID of the file */
161         __be16 FABN;            /* allocation blocks number*/

162 } __packed;

149 struct hfs_cat_key {

150         u8 key_len;             /* number of bytes in the key */
151         u8 reserved;            /* padding */
152         __be32 ParID;           /* CNID of the parent dir */
153         struct hfs_name CName;  /* The filename of the entry */

154 } __packed;

because:

27 #define HFS_NAMELEN 31 /* maximum length of an HFS filename */

87 struct hfs_name {
88 u8 len;
89 u8 name[HFS_NAMELEN];
90 } __packed;

If we are using sizeof(struct hfs_cat_key) then it looks like that we could potentially miss one byte of the union during catalog key copying. But if we will copy struct hfs_ext_key then we will copy some amount of "garbage" anyway. So, I don't think that it's good fix of the issue. What do you think?

Another worry could be the "search_key" field of the struct hfs_find_data.

Thanks,
Vyacheslav Dubeyko.

Dan Carpenter

author

2016-01-26 19:18 UTC #2 lore raw

Hm, I completely didn't see that it was a union instead of a struct. I still think my fix is actually correct though. Now that you point out the union, I see that my change is equivalent to just removing the '&' char.

- memcpy(&rd->key, &fd.key, sizeof(struct hfs_cat_key));
+ memcpy(&rd->key, fd.key, sizeof(struct hfs_cat_key));

We don't want to copy sizeof(*fd.key) because that would write past the end of the destination struct.

Quoted 2 lines · Viacheslav Dubeyko

Another worry could be the "search_key" field of the struct hfs_find_data.

I don't understand what you mean here.

regards,
dan carpenter

Viacheslav Dubeyko

maintainer

2016-01-26 21:54 UTC #3 lore raw

Review

Quoted 8 lines · Dan Carpenter

Hm, I completely didn't see that it was a union instead of a struct. I still think my fix is actually correct though. Now that you point out the union, I see that my change is equivalent to just removing the '&' char.
(quoted diff) +1 -1
-	memcpy(&rd->key, &fd.key, sizeof(struct hfs_cat_key));
+	memcpy(&rd->key, fd.key, sizeof(struct hfs_cat_key));

Yeahh, it looks correct right now. The rd is the pointer that includes struct hfs_cat_key object. So, we need to use &rd->key. But on another side we have struct hfs_find_data object on the stack. And this object includes the pointer on union btree_key. We want to copy struct hfs_cat_key object and we should use sizeof(struct hfs_cat_key).

Quoted 9 lines

We don't want to copy sizeof(*fd.key) because that would write past the end of the destination struct.
Quoted 2 lines · Viacheslav Dubeyko
Another worry could be the "search_key" field of the struct hfs_find_data.
I don't understand what you mean here.

I mean here that we could have another incorrect copy operations for "search_key" field. That's all.

Thanks,
Vyacheslav Dubeyko.

Dan Carpenter

author

2017-01-16 14:22 UTC #4 lore raw

I was reviewing old warnings and I stumbled across this one again. Although I wrote that &fd.key->cat and "fd.key" are equivalent, I feel that actually we should be doing the former. fd.key is a union but we want the ->cat member of the union.

Quoted 6 lines · Viacheslav Dubeyko

Quoted 5 lines · Dan Carpenter
Hm, I completely didn't see that it was a union instead of a struct. I still think my fix is actually correct though. Now that you point out the union, I see that my change is equivalent to just removing the '&' char.

Quoted 9 lines

Review
Quoted 3 lines
(quoted diff) +1 -1
-	memcpy(&rd->key, &fd.key, sizeof(struct hfs_cat_key));
+	memcpy(&rd->key, fd.key, sizeof(struct hfs_cat_key));
Yeahh, it looks correct right now. The rd is the pointer that includes struct hfs_cat_key object. So, we need to use &rd->key. But on another side we have struct hfs_find_data object on the stack. And this object includes the pointer on union btree_key. We want to copy struct hfs_cat_key object and we should use sizeof(struct hfs_cat_key).

I've read this paragraph several times now and I think you are saying that the patch is correct.

Quoted 5 lines

Quoted 5 lines
We don't want to copy sizeof(*fd.key) because that would write past the end of the destination struct.
On Tue, Jan 26, 2016 at 10:18:56AM -0800, Viacheslav Dubeyko wrote:

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Another worry could be the "search_key" field of the struct hfs_find_data.
I don't understand what you mean here.
I mean here that we could have another incorrect copy operations for "search_key" field. That's all.

I don't see the bugs you are saying might exist... ;)

regards,
dan carpenter

Viacheslav Dubeyko

maintainer

2017-01-16 22:34 UTC #5 lore raw

Quoted 30 lines · Dan Carpenter

I was reviewing old warnings and I stumbled across this one again. Although I wrote that &fd.key->cat and "fd.key" are equivalent, I feel
that actually we should be doing the former.  fd.key is a union but we
want the ->cat member of the union.
Quoted 11 lines · Viacheslav Dubeyko
Quoted 9 lines · Dan Carpenter
Hm, I completely didn't see that it was a union instead of a struct.  I
still think my fix is actually correct though.  Now that you point out
the union, I see that my change is equivalent to just removing the '&'
char.
Quoted 11 lines
Review
Quoted 3 lines
(quoted diff) +1 -1
-	memcpy(&rd->key, &fd.key, sizeof(struct hfs_cat_key));
+	memcpy(&rd->key, fd.key, sizeof(struct hfs_cat_key));
Yeahh, it looks correct right now. The rd is the pointer that includes
struct hfs_cat_key object. So, we need to use &rd->key. But on another
side we have struct hfs_find_data object on the stack. And this object
includes the pointer on union btree_key. We want to copy struct hfs_cat_key object and we should use sizeof(struct hfs_cat_key).

Quoted 3 lines

I've read this paragraph several times now and I think you are saying that the patch is correct.

Yes, I've said that patch looks good. I think it's better to resend the patch again.

Thanks,
Vyacheslav Dubeyko.

Dan Carpenter

author

2017-01-18 11:13 UTC #6 lore raw

PATCH RESEND 1/1 hfs: fix fix hfs_readdir()

I was looking through static analysis warnings and there is a bug here that goes all the way back to the start of git. Basically we're copying the pointer and nearby garbage instead of the data the fd.key pointer is pointing to.

Signed-off-by Dan Carpenter <[email protected]>

---
I sent this a year ago, and we had a thread about it, but in the end decided that the original patch was correct. Not tested.

fs/hfs/dir.c +1 -1

index 5de5c48..75b2542 100644
@@ -169,7 +169,7 @@ static int hfs_readdir(struct file *file, struct dir_context *ctx)
 	 * Can be done after the list insertion; exclusion with
 	 * hfs_delete_cat() is provided by directory lock.
 	 */
-	memcpy(&rd->key, &fd.key, sizeof(struct hfs_cat_key));
+	memcpy(&rd->key, &fd.key->cat, sizeof(struct hfs_cat_key));
 out:
 	hfs_find_exit(&fd);
 	return err;

Viacheslav Dubeyko

maintainer Reviewed-by on v1 1/1

2017-01-18 17:28 UTC #7 lore raw

Re: [patch resend] hfs: fix fix hfs_readdir()

Quoted 24 lines · Dan Carpenter

I was looking through static analysis warnings and there is a bug here
that goes all the way back to the start of git. Basically we're copying
the pointer and nearby garbage instead of the data the fd.key pointer is
pointing to.
Signed-off-by Dan Carpenter <[email protected]>
---
I sent this a year ago, and we had a thread about it, but in the end decided that the original patch was correct. Not tested.
fs/hfs/dir.c +1 -1
index 5de5c48..75b2542 100644
@@ -169,7 +169,7 @@ static int hfs_readdir(struct file *file, struct
 dir_context *ctx)
  	 * Can be done after the list insertion; exclusion with
  	 * hfs_delete_cat() is provided by directory lock.
  	 */
-	memcpy(&rd->key, &fd.key, sizeof(struct hfs_cat_key));
+	memcpy(&rd->key, &fd.key->cat, sizeof(struct hfs_cat_key));

Quoted 3 lines

out:
hfs_find_exit(&fd);
return err;

Looks good.

Reviewed-by Vyacheslav Dubeyko <[email protected]>

Thanks,
Vyacheslav Dubeyko.