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");
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
