[patch] hfs: fix hfs_readdir()

[patch] hfs: fix hfs_readdir()

[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 <dan.carpenter@oracle.com>
---
Not tested.  Please review carefully.

diff --git a/fs/hfs/dir.c b/fs/hfs/dir.c
index 70788e0..66485d7 100644
--- a/fs/hfs/dir.c
+++ b/fs/hfs/dir.c
@@ -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;

Re: [patch] hfs: fix hfs_readdir()

[Viacheslav Dubeyko]

On Tue, 2016-01-26 at 12:26 +0300, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> ---
> Not tested.  Please review carefully.
> 
> diff --git a/fs/hfs/dir.c b/fs/hfs/dir.c
> index 70788e0..66485d7 100644
> --- a/fs/hfs/dir.c
> +++ b/fs/hfs/dir.c
> @@ -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.

Re: [patch] hfs: fix hfs_readdir()

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

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

On Tue, Jan 26, 2016 at 10:18:56AM -0800, Viacheslav Dubeyko wrote:
> 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

Re: [patch] hfs: fix hfs_readdir()

[Viacheslav Dubeyko]

On Tue, 2016-01-26 at 22:18 +0300, Dan Carpenter wrote:
> 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));
> 

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

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

Re: [patch] hfs: fix hfs_readdir()

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

On Tue, Jan 26, 2016 at 01:54:06PM -0800, Viacheslav Dubeyko wrote:
> On Tue, 2016-01-26 at 22:18 +0300, Dan Carpenter wrote:
> > 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));
> > 
> 
> 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.

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

Re: [patch] hfs: fix hfs_readdir()

[Viacheslav Dubeyko]

On Mon, 2017-01-16 at 17:22 +0300, Dan Carpenter wrote:
> 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.
> 
> On Tue, Jan 26, 2016 at 01:54:06PM -0800, Viacheslav Dubeyko wrote:
> > 
> > On Tue, 2016-01-26 at 22:18 +0300, Dan Carpenter wrote:
> > > 
> > > 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));
> > > 
> > 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.
> 

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

Thanks,
Vyacheslav Dubeyko.

[patch resend] hfs: fix fix hfs_readdir()

[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 <dan.carpenter@oracle.com>
---
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.

diff --git a/fs/hfs/dir.c b/fs/hfs/dir.c
index 5de5c48..75b2542 100644
--- a/fs/hfs/dir.c
+++ b/fs/hfs/dir.c
@@ -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;

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

[Viacheslav Dubeyko]

On Wed, 2017-01-18 at 14:13 +0300, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> ---
> 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.
> 
> diff --git a/fs/hfs/dir.c b/fs/hfs/dir.c
> index 5de5c48..75b2542 100644
> --- a/fs/hfs/dir.c
> +++ b/fs/hfs/dir.c
> @@ -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;

Looks good.

Reviewed-by: Vyacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Vyacheslav Dubeyko.