fcntl: return -EFAULT if copy_to_user fails
in [Kernel]
|
From: Dan Carpenter on 3 Jun 2010 08:50 On Thu, Jun 03, 2010 at 09:16:52PM +0900, Takuya Yoshikawa wrote: > (2010/06/03 20:59), Jens Axboe wrote: >> On 2010-06-03 12:35, Dan Carpenter wrote: >>> copy_to_user() returns the number of bytes remaining, but we want to >>> return -EFAULT. >>> ret = fcntl(fd, F_SETOWN_EX, NULL); >>> With the original code ret would be 8 here. >>> >>> V2: Takuya Yoshikawa pointed out a similar issue in f_getown_ex() >> >> Pretty basic bug, how long has this been there? > > IIUC, from the beginning, when these were introduced. > > And I recently sent similar bug fixes for other parts. > It was your clear_user() patch which inspired me. I wrote a smatch check to find these. I've pushed the code to the smatch repo. http://repo.or.cz/r/smatch.git The heuristic I use is that if we return a variable which is the return value of copy_to_user() and it's non-zero then complain. It didn't find the f_getown_ex() because that return value could come from copy_to_user() or it could be -EINVAL. I'll mess with it a bit and see if I can make it catch the f_getown_ex() bug. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Al Viro on 3 Jun 2010 08:50 On Thu, Jun 03, 2010 at 07:22:34PM +0900, Takuya Yoshikawa wrote: > (2010/06/03 19:04), Dan Carpenter wrote: > >copy_to_user() returns the number of bytes remaining, but we want to > >return -EFAULT. > > > > ret = fcntl(fd, F_SETOWN_EX, NULL); > > > >With the original code ret would be 8 here. > > > >Signed-off-by: Dan Carpenter<error27(a)gmail.com> > > How about f_getown_ex() ? > > if (!ret) > ret = copy_to_user(owner_p, &owner, sizeof(owner)); > return ret; > > Fixing this too would be better, I think. > > Takuya Applied, will push today. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Nick Piggin on 3 Jun 2010 09:20 On Thu, Jun 03, 2010 at 02:38:03PM +0200, Eric Dumazet wrote: > Le jeudi 03 juin 2010 � 21:16 +0900, Takuya Yoshikawa a �crit : > > (2010/06/03 20:59), Jens Axboe wrote: > > > On 2010-06-03 12:35, Dan Carpenter wrote: > > >> copy_to_user() returns the number of bytes remaining, but we want to > > >> return -EFAULT. > > >> ret = fcntl(fd, F_SETOWN_EX, NULL); > > >> With the original code ret would be 8 here. > > >> > > >> V2: Takuya Yoshikawa pointed out a similar issue in f_getown_ex() > > > > > > Pretty basic bug, how long has this been there? > > > > IIUC, from the beginning, when these were introduced. > > Maybe copy_to_user() was changed sometime to return a partial count > instead of EFAULT ? I think it's been like that since first introduced. Some functions do need to know in order to do partial copies. > I do think we should have a set of helper functions, instead of > spreading special EFAULT cases in one housand places... > > This is really ugly. > > static inline int sec_copy_to_user(arg1, arg2, arg3) > { > int res = copy_to_user(arg1, arg2, arg3); > > return (res > 0) ? -EFAULT : res; > } It would be unfortunate if it adds more confusion. I'd prefer to have a sufficiently different name. memcpy_to_user/memcpy_from_user perhaps? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Takuya Yoshikawa on 3 Jun 2010 09:30 (2010/06/03 22:10), Nick Piggin wrote: > On Thu, Jun 03, 2010 at 02:38:03PM +0200, Eric Dumazet wrote: >> Le jeudi 03 juin 2010 � 21:16 +0900, Takuya Yoshikawa a �crit : >>> (2010/06/03 20:59), Jens Axboe wrote: >>>> On 2010-06-03 12:35, Dan Carpenter wrote: >>>>> copy_to_user() returns the number of bytes remaining, but we want to >>>>> return -EFAULT. >>>>> ret = fcntl(fd, F_SETOWN_EX, NULL); >>>>> With the original code ret would be 8 here. >>>>> >>>>> V2: Takuya Yoshikawa pointed out a similar issue in f_getown_ex() >>>> >>>> Pretty basic bug, how long has this been there? >>> >>> IIUC, from the beginning, when these were introduced. >> >> Maybe copy_to_user() was changed sometime to return a partial count >> instead of EFAULT ? > > I think it's been like that since first introduced. Some functions > do need to know in order to do partial copies. > > >> I do think we should have a set of helper functions, instead of >> spreading special EFAULT cases in one housand places... >> >> This is really ugly. >> >> static inline int sec_copy_to_user(arg1, arg2, arg3) >> { >> int res = copy_to_user(arg1, arg2, arg3); >> >> return (res> 0) ? -EFAULT : res; >> } > > It would be unfortunate if it adds more confusion. I'd prefer to have > a sufficiently different name. memcpy_to_user/memcpy_from_user > perhaps? Then, and memclear_user() ? > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Eric Dumazet on 3 Jun 2010 09:50 Le jeudi 03 juin 2010 à 22:24 +0900, Takuya Yoshikawa a écrit : > (2010/06/03 22:10), Nick Piggin wrote: > > On Thu, Jun 03, 2010 at 02:38:03PM +0200, Eric Dumazet wrote: > >> This is really ugly. > >> > >> static inline int sec_copy_to_user(arg1, arg2, arg3) > >> { > >> int res = copy_to_user(arg1, arg2, arg3); > >> > >> return (res> 0) ? -EFAULT : res; > >> } > > > > It would be unfortunate if it adds more confusion. I'd prefer to have > > a sufficiently different name. memcpy_to_user/memcpy_from_user > > perhaps? > > Then, and memclear_user() ? > > > > We are interested by the fact that full copy is done, so maybe use the 'full' prefix ? fullcopy_to_user() , fullclear_user() ? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
First
|
Prev
|
Next
|
Last
Pages: 1 2 3 Prev: Crypto Fixes for 2.6.35 Next: ipconfig: document DHCP hostname and DNS record |