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

[Eric Biggers <ebiggers@kernel.org>]

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