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 <ebigg...@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 <ebigg...@google.com>
>>>>>
>>>>> Anyway, it makes sense to me.
>>>>>
>>>>> Reviewed-by: Chao Yu <yuch...@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
