| Message ID | 20171114164818.6783-1-lav@etersoft.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 14, 2017 at 07:48:18PM +0300, Vitaly Lipatov wrote: > for fcntl64 with F_GETLK64 we need use checking against COMPAT_LOFF_T_MAX. > > Fixes: 94073ad77fff2 "fs/locks: don't mess with the address limit in compat_fcntl64" > > Signed-off-by: Vitaly Lipatov <lav@etersoft.ru> > --- > fs/fcntl.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/fs/fcntl.c b/fs/fcntl.c > index 30f47d0..e9443d9 100644 > --- a/fs/fcntl.c > +++ b/fs/fcntl.c > @@ -590,17 +590,17 @@ convert_fcntl_cmd(unsigned int cmd) > * GETLK was successful and we need to return the data, but it needs to fit in > * the compat structure. > * l_start shouldn't be too big, unless the original start + end is greater than I assume that should be start + end. > - * COMPAT_OFF_T_MAX, in which case the app was asking for trouble, so we return > + * off_t_max, in which case the app was asking for trouble, so we return > * -EOVERFLOW in that case. It took me a minute to understand. OK, I get it, the application's not supposed to issue a GETLK with offset+len too large, so of course it shouldn't encounter a conflicting lock out there. I don't think that's true, though, thanks to the special interpretation of length 0 in the argument; it looks to me like we can find a conflict with a lock that starts beyond COMPAT_OFF_T_MAX in that case. I guess that's independent of your patch, though. --b. > l_len could be too big, in which case we just > * truncate it, and only allow the app to see that part of the conflicting lock > * that might make sense to it anyway > */ > -static int fixup_compat_flock(struct flock *flock) > +static int fixup_compat_flock(struct flock *flock, loff_t off_t_max) > { > - if (flock->l_start > COMPAT_OFF_T_MAX) > + if (flock->l_start > off_t_max) > return -EOVERFLOW; > - if (flock->l_len > COMPAT_OFF_T_MAX) > - flock->l_len = COMPAT_OFF_T_MAX; > + if (flock->l_len > off_t_max) > + flock->l_len = off_t_max; > return 0; > } > > @@ -631,7 +631,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, unsigned int, cmd, > err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock); > if (err) > break; > - err = fixup_compat_flock(&flock); > + err = fixup_compat_flock(&flock, COMPAT_OFF_T_MAX); > if (err) > return err; > err = put_compat_flock(&flock, compat_ptr(arg)); > @@ -644,7 +644,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, unsigned int, cmd, > err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock); > if (err) > break; > - err = fixup_compat_flock(&flock); > + err = fixup_compat_flock(&flock, COMPAT_LOFF_T_MAX); > if (err) > return err; > err = put_compat_flock64(&flock, compat_ptr(arg)); > -- > 2.10.4
On Tue, 2017-11-14 at 19:48 +0300, Vitaly Lipatov wrote: > for fcntl64 with F_GETLK64 we need use checking against COMPAT_LOFF_T_MAX. > > Fixes: 94073ad77fff2 "fs/locks: don't mess with the address limit in compat_fcntl64" > > Signed-off-by: Vitaly Lipatov <lav@etersoft.ru> > --- > fs/fcntl.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/fs/fcntl.c b/fs/fcntl.c > index 30f47d0..e9443d9 100644 > --- a/fs/fcntl.c > +++ b/fs/fcntl.c > @@ -590,17 +590,17 @@ convert_fcntl_cmd(unsigned int cmd) > * GETLK was successful and we need to return the data, but it needs to fit in > * the compat structure. > * l_start shouldn't be too big, unless the original start + end is greater than > - * COMPAT_OFF_T_MAX, in which case the app was asking for trouble, so we return > + * off_t_max, in which case the app was asking for trouble, so we return > * -EOVERFLOW in that case. l_len could be too big, in which case we just > * truncate it, and only allow the app to see that part of the conflicting lock > * that might make sense to it anyway > */ > -static int fixup_compat_flock(struct flock *flock) > +static int fixup_compat_flock(struct flock *flock, loff_t off_t_max) > { > - if (flock->l_start > COMPAT_OFF_T_MAX) > + if (flock->l_start > off_t_max) > return -EOVERFLOW; > - if (flock->l_len > COMPAT_OFF_T_MAX) > - flock->l_len = COMPAT_OFF_T_MAX; > + if (flock->l_len > off_t_max) > + flock->l_len = off_t_max; > return 0; > } > Wait... Does this do anything at all in the case where you pass in COMPAT_LOFF_T_MAX? l_start and l_len are either off_t or loff_t (depending on arch). Either one will fit in the F_GETLK64/F_OFD_GETLK struct, so I don't see a need to check here. > > @@ -631,7 +631,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, unsigned int, cmd, > err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock); > if (err) > break; > - err = fixup_compat_flock(&flock); > + err = fixup_compat_flock(&flock, COMPAT_OFF_T_MAX); > if (err) > return err; > err = put_compat_flock(&flock, compat_ptr(arg)); > @@ -644,7 +644,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, unsigned int, cmd, > err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock); > if (err) > break; > - err = fixup_compat_flock(&flock); > + err = fixup_compat_flock(&flock, COMPAT_LOFF_T_MAX); > if (err) > return err; > err = put_compat_flock64(&flock, compat_ptr(arg)); Maybe a simpler fix would be to just remove the fixup_compat_flock call above? PS: if you send any more patches, please cc Christoph. He did the earlier work on cleaning up the compat syscall code, and I'd like him to be kept in the loop on this as well. Thanks,
Jeff Layton писал 14.11.17 22:12: ... > Wait... > > Does this do anything at all in the case where you pass in > COMPAT_LOFF_T_MAX? l_start and l_len are either off_t or loff_t > (depending on arch). > > Either one will fit in the F_GETLK64/F_OFD_GETLK struct, so I don't see > a need to check here. I am not sure, can off_t be bigger than loff_t ? If not, we have just skip checking against COMPAT_LOFF_T_MAX. ... >> @@ -644,7 +644,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, >> unsigned int, cmd, >> err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock); >> if (err) >> break; >> - err = fixup_compat_flock(&flock); >> + err = fixup_compat_flock(&flock, COMPAT_LOFF_T_MAX); >> if (err) >> return err; >> err = put_compat_flock64(&flock, compat_ptr(arg)); > > Maybe a simpler fix would be to just remove the fixup_compat_flock call > above? > > PS: if you send any more patches, please cc Christoph. He did the Ok.
On Tue, 2017-11-14 at 22:25 +0300, Vitaly Lipatov wrote: > Jeff Layton писал 14.11.17 22:12: > ... > > Wait... > > > > Does this do anything at all in the case where you pass in > > COMPAT_LOFF_T_MAX? l_start and l_len are either off_t or loff_t > > (depending on arch). > > > > Either one will fit in the F_GETLK64/F_OFD_GETLK struct, so I don't see > > a need to check here. > > I am not sure, can off_t be bigger than loff_t ? I don't think so, at least not in any possible situation we care about here. > If not, we have just skip checking against COMPAT_LOFF_T_MAX. > > ... > > > @@ -644,7 +644,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, > > > unsigned int, cmd, > > > err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock); > > > if (err) > > > break; > > > - err = fixup_compat_flock(&flock); > > > + err = fixup_compat_flock(&flock, COMPAT_LOFF_T_MAX); > > > if (err) > > > return err; > > > err = put_compat_flock64(&flock, compat_ptr(arg)); > > > > Maybe a simpler fix would be to just remove the fixup_compat_flock call > > above? > > Ok. If you have a test for this, mind testing and sending a patch? Thanks,
Jeff Layton писал 14.11.17 23:19: > On Tue, 2017-11-14 at 22:25 +0300, Vitaly Lipatov wrote: >> Jeff Layton писал 14.11.17 22:12: >> ... >> > Wait... >> > >> > Does this do anything at all in the case where you pass in >> > COMPAT_LOFF_T_MAX? l_start and l_len are either off_t or loff_t >> > (depending on arch). >> > >> > Either one will fit in the F_GETLK64/F_OFD_GETLK struct, so I don't see >> > a need to check here. >> >> I am not sure, can off_t be bigger than loff_t ? > > I don't think so, at least not in any possible situation we care about > here. We have this checking for ages: if (f.l_start > COMPAT_LOFF_T_MAX) ret = -EOVERFLOW; http://debian.securedservers.com/kernel/pub/linux/kernel/people/akpm/patches/2.6/2.6.15-rc5/2.6.15-rc5-mm1/broken-out/fix-overflow-tests-for-compat_sys_fcntl64-locking.patch > >> If not, we have just skip checking against COMPAT_LOFF_T_MAX. >> >> ... >> > > @@ -644,7 +644,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, >> > > unsigned int, cmd, >> > > err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock); >> > > if (err) >> > > break; >> > > - err = fixup_compat_flock(&flock); >> > > + err = fixup_compat_flock(&flock, COMPAT_LOFF_T_MAX); >> > > if (err) >> > > return err; >> > > err = put_compat_flock64(&flock, compat_ptr(arg)); >> > >> > Maybe a simpler fix would be to just remove the fixup_compat_flock call >> > above? >> > > > Ok. If you have a test for this, mind testing and sending a patch? I think if COMPAT_LOFF_T_MAX is exists, that value can be smaller than can fit in off_t. Checking against COMPAT_LOFF_T_MAX keep old logic works for me last 10 years. I have some tests around wine project I worked on. May be later I will do additional tests.
On Wed, 2017-11-15 at 00:22 +0300, Vitaly Lipatov wrote: > Jeff Layton писал 14.11.17 23:19: > > On Tue, 2017-11-14 at 22:25 +0300, Vitaly Lipatov wrote: > > > Jeff Layton писал 14.11.17 22:12: > > > ... > > > > Wait... > > > > > > > > Does this do anything at all in the case where you pass in > > > > COMPAT_LOFF_T_MAX? l_start and l_len are either off_t or loff_t > > > > (depending on arch). > > > > > > > > Either one will fit in the F_GETLK64/F_OFD_GETLK struct, so I don't see > > > > a need to check here. > > > > > > I am not sure, can off_t be bigger than loff_t ? > > > > I don't think so, at least not in any possible situation we care about > > here. > > We have this checking for ages: > if (f.l_start > COMPAT_LOFF_T_MAX) > ret = -EOVERFLOW; > http://debian.securedservers.com/kernel/pub/linux/kernel/people/akpm/patches/2.6/2.6.15-rc5/2.6.15-rc5-mm1/broken-out/fix-overflow-tests-for-compat_sys_fcntl64-locking.patch > I'm not convinced that those checks ever did anything, tbh. > > > > > If not, we have just skip checking against COMPAT_LOFF_T_MAX. > > > > > > ... > > > > > @@ -644,7 +644,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, > > > > > unsigned int, cmd, > > > > > err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock); > > > > > if (err) > > > > > break; > > > > > - err = fixup_compat_flock(&flock); > > > > > + err = fixup_compat_flock(&flock, COMPAT_LOFF_T_MAX); > > > > > if (err) > > > > > return err; > > > > > err = put_compat_flock64(&flock, compat_ptr(arg)); > > > > > > > > Maybe a simpler fix would be to just remove the fixup_compat_flock call > > > > above? > > > > > > > > Ok. If you have a test for this, mind testing and sending a patch? > > I think if COMPAT_LOFF_T_MAX is exists, that value can be smaller than > can fit in off_t. > Checking against COMPAT_LOFF_T_MAX keep old logic works for me last 10 > years. > > I have some tests around wine project I worked on. May be later I will > do additional tests. > I am making an assumption here that l_start and l_end can never be larger than a signed 64-bit value. I don't see how it ever could be, given that it's defined as a long long, but I suppose we could add some exotic arch later that does something weird. Maybe we can just add a BUILD_BUG_ON for that? I'll send along an alternate patch in a few mins.
diff --git a/fs/fcntl.c b/fs/fcntl.c index 30f47d0..e9443d9 100644 --- a/fs/fcntl.c +++ b/fs/fcntl.c @@ -590,17 +590,17 @@ convert_fcntl_cmd(unsigned int cmd) * GETLK was successful and we need to return the data, but it needs to fit in * the compat structure. * l_start shouldn't be too big, unless the original start + end is greater than - * COMPAT_OFF_T_MAX, in which case the app was asking for trouble, so we return + * off_t_max, in which case the app was asking for trouble, so we return * -EOVERFLOW in that case. l_len could be too big, in which case we just * truncate it, and only allow the app to see that part of the conflicting lock * that might make sense to it anyway */ -static int fixup_compat_flock(struct flock *flock) +static int fixup_compat_flock(struct flock *flock, loff_t off_t_max) { - if (flock->l_start > COMPAT_OFF_T_MAX) + if (flock->l_start > off_t_max) return -EOVERFLOW; - if (flock->l_len > COMPAT_OFF_T_MAX) - flock->l_len = COMPAT_OFF_T_MAX; + if (flock->l_len > off_t_max) + flock->l_len = off_t_max; return 0; } @@ -631,7 +631,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, unsigned int, cmd, err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock); if (err) break; - err = fixup_compat_flock(&flock); + err = fixup_compat_flock(&flock, COMPAT_OFF_T_MAX); if (err) return err; err = put_compat_flock(&flock, compat_ptr(arg)); @@ -644,7 +644,7 @@ COMPAT_SYSCALL_DEFINE3(fcntl64, unsigned int, fd, unsigned int, cmd, err = fcntl_getlk(f.file, convert_fcntl_cmd(cmd), &flock); if (err) break; - err = fixup_compat_flock(&flock); + err = fixup_compat_flock(&flock, COMPAT_LOFF_T_MAX); if (err) return err; err = put_compat_flock64(&flock, compat_ptr(arg));
for fcntl64 with F_GETLK64 we need use checking against COMPAT_LOFF_T_MAX. Fixes: 94073ad77fff2 "fs/locks: don't mess with the address limit in compat_fcntl64" Signed-off-by: Vitaly Lipatov <lav@etersoft.ru> --- fs/fcntl.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)