| Message ID | 9028dc83-82a2-fc51-b559-0020b2c0a892@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Avoid crash in epoll_ctl with EPOLL_CTL_DEL | expand |
Le 30/05/2019 à 17:25, Giuseppe Musacchio a écrit : > The `event` parameter is ignored by the kernel if `op` is EPOLL_CTL_DEL, > do the same and avoid returning EFAULT if garbage is passed instead of a > valid pointer. > > Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com> > --- > linux-user/syscall.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 5e29e675e9..32d463d58d 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -11329,7 +11329,7 @@ static abi_long do_syscall1(void *cpu_env, int > num, abi_long arg1, > { > struct epoll_event ep; > struct epoll_event *epp = 0; > - if (arg4) { > + if (arg2 != EPOLL_CTL_DEL && arg4) { > struct target_epoll_event *target_ep; > if (!lock_user_struct(VERIFY_READ, target_ep, arg4, 1)) { > return -TARGET_EFAULT; Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Le 30/05/2019 à 18:00, Laurent Vivier a écrit : > Le 30/05/2019 à 17:25, Giuseppe Musacchio a écrit : >> The `event` parameter is ignored by the kernel if `op` is EPOLL_CTL_DEL, >> do the same and avoid returning EFAULT if garbage is passed instead of a >> valid pointer. >> >> Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com> >> --- >> linux-user/syscall.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >> index 5e29e675e9..32d463d58d 100644 >> --- a/linux-user/syscall.c >> +++ b/linux-user/syscall.c >> @@ -11329,7 +11329,7 @@ static abi_long do_syscall1(void *cpu_env, int >> num, abi_long arg1, >> { >> struct epoll_event ep; >> struct epoll_event *epp = 0; >> - if (arg4) { >> + if (arg2 != EPOLL_CTL_DEL && arg4) { >> struct target_epoll_event *target_ep; >> if (!lock_user_struct(VERIFY_READ, target_ep, arg4, 1)) { >> return -TARGET_EFAULT; > > Reviewed-by: Laurent Vivier <laurent@vivier.eu> > in fact, the BUGS section of epoll_ctl(2) says: "In kernel versions before 2.6.9, the EPOLL_CTL_DEL operation required a non-null pointer in event, even though this argument is ignored. Since Linux 2.6.9, event can be specified as NULL when using EPOLL_CTL_DEL. Applications that need to be portable to kernels before 2.6.9 should specify a non-null pointer in event." So something like this would be more portable: @@ -11329,6 +11329,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, struct epoll_event ep; struct epoll_event *epp = 0; if (arg4) { + if (arg2 != EPOLL_CTL_DEL) { struct target_epoll_event *target_ep; if (!lock_user_struct(VERIFY_READ, target_ep, arg4, 1)) { return -TARGET_EFAULT; @@ -11340,6 +11341,11 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, */ ep.data.u64 = tswap64(target_ep->data.u64); unlock_user_struct(target_ep, arg4, 0); + } + /* + * before kernel 2.6.9, EPOLL_CTL_DEL operation required a + * non-null pointer, even though this argument is ignored. + * */ epp = &ep; } Thanks, Laurent
Yes, that's much better if compatibility with such an old kernel version is wanted. I suppose there's no need for me to re-send the patch. On 30/05/19 18:12, Laurent Vivier wrote: > Le 30/05/2019 à 18:00, Laurent Vivier a écrit : >> Le 30/05/2019 à 17:25, Giuseppe Musacchio a écrit : >>> The `event` parameter is ignored by the kernel if `op` is EPOLL_CTL_DEL, >>> do the same and avoid returning EFAULT if garbage is passed instead of a >>> valid pointer. >>> >>> Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com> >>> --- >>> linux-user/syscall.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >>> index 5e29e675e9..32d463d58d 100644 >>> --- a/linux-user/syscall.c >>> +++ b/linux-user/syscall.c >>> @@ -11329,7 +11329,7 @@ static abi_long do_syscall1(void *cpu_env, int >>> num, abi_long arg1, >>> { >>> struct epoll_event ep; >>> struct epoll_event *epp = 0; >>> - if (arg4) { >>> + if (arg2 != EPOLL_CTL_DEL && arg4) { >>> struct target_epoll_event *target_ep; >>> if (!lock_user_struct(VERIFY_READ, target_ep, arg4, 1)) { >>> return -TARGET_EFAULT; >> >> Reviewed-by: Laurent Vivier <laurent@vivier.eu> >> > > in fact, the BUGS section of epoll_ctl(2) says: > > "In kernel versions before 2.6.9, the EPOLL_CTL_DEL operation required a > non-null pointer in event, even though this argument is ignored. Since > Linux 2.6.9, event can be specified as NULL when using EPOLL_CTL_DEL. > Applications that need to be portable to kernels before 2.6.9 should > specify a non-null pointer in event." > > So something like this would be more portable: > > @@ -11329,6 +11329,7 @@ static abi_long do_syscall1(void *cpu_env, int > num, abi_long arg1, > struct epoll_event ep; > struct epoll_event *epp = 0; > if (arg4) { > + if (arg2 != EPOLL_CTL_DEL) { > struct target_epoll_event *target_ep; > if (!lock_user_struct(VERIFY_READ, target_ep, arg4, 1)) { > return -TARGET_EFAULT; > @@ -11340,6 +11341,11 @@ static abi_long do_syscall1(void *cpu_env, int > num, abi_long arg1, > */ > ep.data.u64 = tswap64(target_ep->data.u64); > unlock_user_struct(target_ep, arg4, 0); > + } > + /* > + * before kernel 2.6.9, EPOLL_CTL_DEL operation required a > + * non-null pointer, even though this argument is ignored. > + * */ > epp = &ep; > } > > Thanks, > Laurent >
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 5e29e675e9..32d463d58d 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -11329,7 +11329,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, { struct epoll_event ep; struct epoll_event *epp = 0; - if (arg4) { + if (arg2 != EPOLL_CTL_DEL && arg4) { struct target_epoll_event *target_ep; if (!lock_user_struct(VERIFY_READ, target_ep, arg4, 1)) {
The `event` parameter is ignored by the kernel if `op` is EPOLL_CTL_DEL, do the same and avoid returning EFAULT if garbage is passed instead of a valid pointer. Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com> --- linux-user/syscall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) return -TARGET_EFAULT;