[f2fs-dev] [PATCH 0/3] f2fs: fixes for FS_IOC_{GET,SET}FSLABEL

[f2fs-dev] [PATCH 0/3] f2fs: fixes for FS_IOC_{GET,SET}FSLABEL

[Eric Biggers]

Fix some bugs in the f2fs implementation of the FS_IOC_GETFSLABEL and
FS_IOC_SETFSLABEL ioctls.

Eric Biggers (3):
  f2fs: fix buffer overruns in FS_IOC_{GET,SET}FSLABEL
  f2fs: fix copying too many bytes in FS_IOC_SETFSLABEL
  f2fs: add missing authorization check in FS_IOC_SETFSLABEL

 fs/f2fs/file.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

-- 
2.22.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH 1/3] f2fs: fix buffer overruns in FS_IOC_{GET, SET}FSLABEL

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

utf16s_to_utf8s() and utf8s_to_utf16s() take the number of characters,
not the number of bytes.

Fixes: 61a3da4d5ef8 ("f2fs: support FS_IOC_{GET,SET}FSLABEL")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/f2fs/file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index eb1aa9b75eda..d521a582d94d 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3094,7 +3094,7 @@ static int f2fs_get_volume_name(struct file *filp, unsigned long arg)
 
 	down_read(&sbi->sb_lock);
 	count = utf16s_to_utf8s(sbi->raw_super->volume_name,
-			sizeof(sbi->raw_super->volume_name),
+			ARRAY_SIZE(sbi->raw_super->volume_name),
 			UTF16_LITTLE_ENDIAN, vbuf, MAX_VOLUME_NAME);
 	up_read(&sbi->sb_lock);
 
@@ -3139,7 +3139,7 @@ static int f2fs_set_volume_name(struct file *filp, unsigned long arg)
 			sizeof(sbi->raw_super->volume_name));
 	utf8s_to_utf16s(vbuf, MAX_VOLUME_NAME, UTF16_LITTLE_ENDIAN,
 			sbi->raw_super->volume_name,
-			sizeof(sbi->raw_super->volume_name));
+			ARRAY_SIZE(sbi->raw_super->volume_name));
 
 	err = f2fs_commit_super(sbi, false);
 
-- 
2.22.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH 2/3] f2fs: fix copying too many bytes in FS_IOC_SETFSLABEL

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Userspace provides a null-terminated string, so don't assume that the
full FSLABEL_MAX bytes can always be copied.

Fixes: 61a3da4d5ef8 ("f2fs: support FS_IOC_{GET,SET}FSLABEL")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/f2fs/file.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index d521a582d94d..315127251bc1 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3111,23 +3111,11 @@ static int f2fs_set_volume_name(struct file *filp, unsigned long arg)
 	struct inode *inode = file_inode(filp);
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	char *vbuf;
-	int len;
 	int err = 0;
 
-	vbuf = f2fs_kzalloc(sbi, MAX_VOLUME_NAME, GFP_KERNEL);
-	if (!vbuf)
-		return -ENOMEM;
-
-	if (copy_from_user(vbuf, (char __user *)arg, FSLABEL_MAX)) {
-		err = -EFAULT;
-		goto out;
-	}
-
-	len = strnlen(vbuf, FSLABEL_MAX);
-	if (len > FSLABEL_MAX - 1) {
-		err = -EINVAL;
-		goto out;
-	}
+	vbuf = strndup_user((const char __user *)arg, FSLABEL_MAX);
+	if (IS_ERR(vbuf))
+		return PTR_ERR(vbuf);
 
 	err = mnt_want_write_file(filp);
 	if (err)
@@ -3137,7 +3125,7 @@ static int f2fs_set_volume_name(struct file *filp, unsigned long arg)
 
 	memset(sbi->raw_super->volume_name, 0,
 			sizeof(sbi->raw_super->volume_name));
-	utf8s_to_utf16s(vbuf, MAX_VOLUME_NAME, UTF16_LITTLE_ENDIAN,
+	utf8s_to_utf16s(vbuf, strlen(vbuf), UTF16_LITTLE_ENDIAN,
 			sbi->raw_super->volume_name,
 			ARRAY_SIZE(sbi->raw_super->volume_name));
 
@@ -3147,7 +3135,7 @@ static int f2fs_set_volume_name(struct file *filp, unsigned long arg)
 
 	mnt_drop_write_file(filp);
 out:
-	kvfree(vbuf);
+	kfree(vbuf);
 	return err;
 }
 
-- 
2.22.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH 3/3] f2fs: add missing authorization check in FS_IOC_SETFSLABEL

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

FS_IOC_SETFSLABEL modifies the filesystem superblock, so it shouldn't be
allowed to regular users.  Require CAP_SYS_ADMIN, like xfs and btrfs do.

Fixes: 61a3da4d5ef8 ("f2fs: support FS_IOC_{GET,SET}FSLABEL")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/f2fs/file.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 315127251bc1..6939cddc542a 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3113,6 +3113,9 @@ static int f2fs_set_volume_name(struct file *filp, unsigned long arg)
 	char *vbuf;
 	int err = 0;
 
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
 	vbuf = strndup_user((const char __user *)arg, FSLABEL_MAX);
 	if (IS_ERR(vbuf))
 		return PTR_ERR(vbuf);
-- 
2.22.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 0/3] f2fs: fixes for FS_IOC_{GET,SET}FSLABEL

[Chao Yu]

Eric, thanks for all the fixes.

Thanks,

On 2019/8/16 13:55, Eric Biggers wrote:
> Fix some bugs in the f2fs implementation of the FS_IOC_GETFSLABEL and
> FS_IOC_SETFSLABEL ioctls.
> 
> Eric Biggers (3):
>   f2fs: fix buffer overruns in FS_IOC_{GET,SET}FSLABEL
>   f2fs: fix copying too many bytes in FS_IOC_SETFSLABEL
>   f2fs: add missing authorization check in FS_IOC_SETFSLABEL
> 
>  fs/f2fs/file.c | 27 +++++++++------------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 1/3] f2fs: fix buffer overruns in FS_IOC_{GET, SET}FSLABEL

[Chao Yu]

On 2019/8/16 13:55, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> utf16s_to_utf8s() and utf8s_to_utf16s() take the number of characters,
> not the number of bytes.
> 
> Fixes: 61a3da4d5ef8 ("f2fs: support FS_IOC_{GET,SET}FSLABEL")
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/3] f2fs: fix copying too many bytes in FS_IOC_SETFSLABEL

[Chao Yu]

On 2019/8/16 13:55, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Userspace provides a null-terminated string, so don't assume that the
> full FSLABEL_MAX bytes can always be copied.>
> Fixes: 61a3da4d5ef8 ("f2fs: support FS_IOC_{GET,SET}FSLABEL")

It may only copy redundant zero bytes, and will not hit security issue, it
doesn't look like a bug fix?

> Signed-off-by: Eric Biggers <ebiggers@google.com>

Anyway, it makes sense to me.

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 3/3] f2fs: add missing authorization check in FS_IOC_SETFSLABEL

[Chao Yu]

On 2019/8/16 13:55, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> FS_IOC_SETFSLABEL modifies the filesystem superblock, so it shouldn't be
> allowed to regular users.  Require CAP_SYS_ADMIN, like xfs and btrfs do.
> 
> Fixes: 61a3da4d5ef8 ("f2fs: support FS_IOC_{GET,SET}FSLABEL")
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 0/3] f2fs: fixes for FS_IOC_{GET,SET}FSLABEL

[Chao Yu]

Hi Eric,

Do you mind folding these fixes into original patch as it's still in dev branch?

BTW, any comment or suggestion is welcome in my original patch.... :)

Thanks,

On 2019-8-16 14:49, Chao Yu wrote:
> Eric, thanks for all the fixes.
> 
> Thanks,
> 
> On 2019/8/16 13:55, Eric Biggers wrote:
>> Fix some bugs in the f2fs implementation of the FS_IOC_GETFSLABEL and
>> FS_IOC_SETFSLABEL ioctls.
>>
>> Eric Biggers (3):
>>   f2fs: fix buffer overruns in FS_IOC_{GET,SET}FSLABEL
>>   f2fs: fix copying too many bytes in FS_IOC_SETFSLABEL
>>   f2fs: add missing authorization check in FS_IOC_SETFSLABEL
>>
>>  fs/f2fs/file.c | 27 +++++++++------------------
>>  1 file changed, 9 insertions(+), 18 deletions(-)
>>
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/3] f2fs: fix copying too many bytes in FS_IOC_SETFSLABEL

[Eric Biggers]

On Fri, Aug 16, 2019 at 02:59:37PM +0800, Chao Yu wrote:
> On 2019/8/16 13:55, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Userspace provides a null-terminated string, so don't assume that the
> > full FSLABEL_MAX bytes can always be copied.>
> > Fixes: 61a3da4d5ef8 ("f2fs: support FS_IOC_{GET,SET}FSLABEL")
> 
> It may only copy redundant zero bytes, and will not hit security issue, it
> doesn't look like a bug fix?
> 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> 
> Anyway, it makes sense to me.
> 
> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> 

It's not clear that userspace is guaranteed to provide a full FSLABEL_MAX bytes
in the buffer.  E.g. it could provide "foo\0" followed by an unmapped page.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 0/3] f2fs: fixes for FS_IOC_{GET,SET}FSLABEL

[Eric Biggers]

On Sun, Aug 18, 2019 at 09:03:21PM +0800, Chao Yu wrote:
> Hi Eric,
> 
> Do you mind folding these fixes into original patch as it's still in dev branch?
> 
> BTW, any comment or suggestion is welcome in my original patch.... :)
> 
> Thanks,
> 

I'm fine with the patches being folded in.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/3] f2fs: fix copying too many bytes in FS_IOC_SETFSLABEL

[Chao Yu]

On 2019/8/18 23:41, Eric Biggers wrote:
> On Fri, Aug 16, 2019 at 02:59:37PM +0800, Chao Yu wrote:
>> On 2019/8/16 13:55, Eric Biggers wrote:
>>> From: Eric Biggers <ebiggers@google.com>
>>>
>>> Userspace provides a null-terminated string, so don't assume that the
>>> full FSLABEL_MAX bytes can always be copied.>
>>> Fixes: 61a3da4d5ef8 ("f2fs: support FS_IOC_{GET,SET}FSLABEL")
>>
>> It may only copy redundant zero bytes, and will not hit security issue, it
>> doesn't look like a bug fix?
>>
>>> Signed-off-by: Eric Biggers <ebiggers@google.com>
>>
>> Anyway, it makes sense to me.
>>
>> Reviewed-by: Chao Yu <yuchao0@huawei.com>
>>
> 
> It's not clear that userspace is guaranteed to provide a full FSLABEL_MAX bytes
> in the buffer.  E.g. it could provide "foo\0" followed by an unmapped page.

You're right, thanks for your explanation.

Thanks,

> 
> - Eric
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/3] f2fs: fix copying too many bytes in FS_IOC_SETFSLABEL

[Chao Yu]

On 2019/8/19 9:33, Chao Yu wrote:
> On 2019/8/18 23:41, Eric Biggers wrote:
>> On Fri, Aug 16, 2019 at 02:59:37PM +0800, Chao Yu wrote:
>>> On 2019/8/16 13:55, Eric Biggers wrote:
>>>> From: Eric Biggers <ebiggers@google.com>
>>>>
>>>> Userspace provides a null-terminated string, so don't assume that the
>>>> full FSLABEL_MAX bytes can always be copied.>
>>>> Fixes: 61a3da4d5ef8 ("f2fs: support FS_IOC_{GET,SET}FSLABEL")
>>>
>>> It may only copy redundant zero bytes, and will not hit security issue, it
>>> doesn't look like a bug fix?
>>>
>>>> Signed-off-by: Eric Biggers <ebiggers@google.com>
>>>
>>> Anyway, it makes sense to me.
>>>
>>> Reviewed-by: Chao Yu <yuchao0@huawei.com>
>>>
>>
>> It's not clear that userspace is guaranteed to provide a full FSLABEL_MAX bytes
>> in the buffer.  E.g. it could provide "foo\0" followed by an unmapped page.
> 
> You're right, thanks for your explanation.

One more question, there is no validation check on length of user passed buffer,

So in most ioctl interfaces, user can pass a buffer which has less size than we
defined intentionally/unintentionally.

E.g.

user space:

struct f2fs_defragment_user {
	unsigned long long start;
//	unsigned long long len;
};

main()
{
	struct f2fs_defragment_user *df;

	df = malloc();
	
	ioctl(fd, F2FS_IOC_DEFRAGMENT, df);
}

kernel:

f2fs_ioc_defragment()
{
...
	if (copy_from_user(&range, (struct f2fs_defragment __user *)arg,
							sizeof(range)))
		return -EFAULT;
}

Is that a common issue?

Thanks,

> 
> Thanks,
> 
>>
>> - Eric
>> .
>>
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/3] f2fs: fix copying too many bytes in FS_IOC_SETFSLABEL

[Eric Biggers]

On Mon, Aug 19, 2019 at 09:58:30AM +0800, Chao Yu wrote:
> On 2019/8/19 9:33, Chao Yu wrote:
> > On 2019/8/18 23:41, Eric Biggers wrote:
> >> On Fri, Aug 16, 2019 at 02:59:37PM +0800, Chao Yu wrote:
> >>> On 2019/8/16 13:55, Eric Biggers wrote:
> >>>> From: Eric Biggers <ebiggers@google.com>
> >>>>
> >>>> Userspace provides a null-terminated string, so don't assume that the
> >>>> full FSLABEL_MAX bytes can always be copied.>
> >>>> Fixes: 61a3da4d5ef8 ("f2fs: support FS_IOC_{GET,SET}FSLABEL")
> >>>
> >>> It may only copy redundant zero bytes, and will not hit security issue, it
> >>> doesn't look like a bug fix?
> >>>
> >>>> Signed-off-by: Eric Biggers <ebiggers@google.com>
> >>>
> >>> Anyway, it makes sense to me.
> >>>
> >>> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> >>>
> >>
> >> It's not clear that userspace is guaranteed to provide a full FSLABEL_MAX bytes
> >> in the buffer.  E.g. it could provide "foo\0" followed by an unmapped page.
> > 
> > You're right, thanks for your explanation.
> 
> One more question, there is no validation check on length of user passed buffer,
> 
> So in most ioctl interfaces, user can pass a buffer which has less size than we
> defined intentionally/unintentionally.
> 
> E.g.
> 
> user space:
> 
> struct f2fs_defragment_user {
> 	unsigned long long start;
> //	unsigned long long len;
> };
> 
> main()
> {
> 	struct f2fs_defragment_user *df;
> 
> 	df = malloc();
> 	
> 	ioctl(fd, F2FS_IOC_DEFRAGMENT, df);
> }
> 
> kernel:
> 
> f2fs_ioc_defragment()
> {
> ...
> 	if (copy_from_user(&range, (struct f2fs_defragment __user *)arg,
> 							sizeof(range)))
> 		return -EFAULT;
> }
> 
> Is that a common issue?
> 

No, but that's different because that only involves a fixed-length struct.

My concern was that since FS_IOC_SETFSLABEL takes in a string, users might do:

	ioctl(fd, FS_IOC_SETFSLABEL, "foo");

Rather than:

	char label[FSLABEL_MAX] = "foo";

	ioctl(fd, FS_IOC_SETFSLABEL, label);

At least that's how I understand the ioctl; AFAICS it does not have a man page,
so I'm not sure what was intended.  Assuming the buffer is always FSLABEL_MAX
bytes seems like a really bad idea though, since if users pass a conventional
string (as is the natural thing to do; open() doesn't require a buffer of length
PATH_MAX, for example...) it will succeed/fail at random depending on whether
the following page is mapped or not.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 0/3] f2fs: fixes for FS_IOC_{GET,SET}FSLABEL

[Jaegeuk Kim]

On 08/18, Eric Biggers wrote:
> On Sun, Aug 18, 2019 at 09:03:21PM +0800, Chao Yu wrote:
> > Hi Eric,
> > 
> > Do you mind folding these fixes into original patch as it's still in dev branch?
> > 
> > BTW, any comment or suggestion is welcome in my original patch.... :)
> > 
> > Thanks,
> > 
> 
> I'm fine with the patches being folded in.

Okay, let me fold them.

> 
> - Eric
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/3] f2fs: fix copying too many bytes in FS_IOC_SETFSLABEL

[Chao Yu]

On 2019/8/19 10:55, Eric Biggers wrote:
> On Mon, Aug 19, 2019 at 09:58:30AM +0800, Chao Yu wrote:
>> On 2019/8/19 9:33, Chao Yu wrote:
>>> On 2019/8/18 23:41, Eric Biggers wrote:
>>>> On Fri, Aug 16, 2019 at 02:59:37PM +0800, Chao Yu wrote:
>>>>> On 2019/8/16 13:55, Eric Biggers wrote:
>>>>>> From: Eric Biggers <ebiggers@google.com>
>>>>>>
>>>>>> Userspace provides a null-terminated string, so don't assume that the
>>>>>> full FSLABEL_MAX bytes can always be copied.>
>>>>>> Fixes: 61a3da4d5ef8 ("f2fs: support FS_IOC_{GET,SET}FSLABEL")
>>>>>
>>>>> It may only copy redundant zero bytes, and will not hit security issue, it
>>>>> doesn't look like a bug fix?
>>>>>
>>>>>> Signed-off-by: Eric Biggers <ebiggers@google.com>
>>>>>
>>>>> Anyway, it makes sense to me.
>>>>>
>>>>> Reviewed-by: Chao Yu <yuchao0@huawei.com>
>>>>>
>>>>
>>>> It's not clear that userspace is guaranteed to provide a full FSLABEL_MAX bytes
>>>> in the buffer.  E.g. it could provide "foo\0" followed by an unmapped page.
>>>
>>> You're right, thanks for your explanation.
>>
>> One more question, there is no validation check on length of user passed buffer,
>>
>> So in most ioctl interfaces, user can pass a buffer which has less size than we
>> defined intentionally/unintentionally.
>>
>> E.g.
>>
>> user space:
>>
>> struct f2fs_defragment_user {
>> 	unsigned long long start;
>> //	unsigned long long len;
>> };
>>
>> main()
>> {
>> 	struct f2fs_defragment_user *df;
>>
>> 	df = malloc();
>> 	
>> 	ioctl(fd, F2FS_IOC_DEFRAGMENT, df);
>> }
>>
>> kernel:
>>
>> f2fs_ioc_defragment()
>> {
>> ...
>> 	if (copy_from_user(&range, (struct f2fs_defragment __user *)arg,
>> 							sizeof(range)))
>> 		return -EFAULT;
>> }
>>
>> Is that a common issue?
>>
> 
> No, but that's different because that only involves a fixed-length struct.
> 
> My concern was that since FS_IOC_SETFSLABEL takes in a string, users might do:
> 
> 	ioctl(fd, FS_IOC_SETFSLABEL, "foo");

Yes, I can understand your concern, since in this case, user's behavior is normal.

But what I'm trying to say is, from the result aspect, when user pass a buffer
which has less size intentionally/unintentionally (even kernel defines a
fix-sized struture, but there is no rules that users can not reconstruct it or
change its size as their wish), kernel may access unmapped page follows to
user's buffer potentially. It needs to be fixed if that's a real problem.

Thanks,

> 
> Rather than:
> 
> 	char label[FSLABEL_MAX] = "foo";
> 
> 	ioctl(fd, FS_IOC_SETFSLABEL, label);
> 
> At least that's how I understand the ioctl; AFAICS it does not have a man page,
> so I'm not sure what was intended.  Assuming the buffer is always FSLABEL_MAX
> bytes seems like a really bad idea though, since if users pass a conventional
> string (as is the natural thing to do; open() doesn't require a buffer of length
> PATH_MAX, for example...) it will succeed/fail at random depending on whether
> the following page is mapped or not.
> 
> - Eric
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel