* Re: [PATCH 2/2] linux-user: Allow sendmsg() without IOV
2022-12-12 17:34 ` [PATCH 2/2] linux-user: Allow sendmsg() without IOV Helge Deller
@ 2023-01-31 12:28 ` Laurent Vivier
2023-01-31 13:34 ` Helge Deller
2023-01-31 15:17 ` Laurent Vivier
2023-01-31 15:19 ` Laurent Vivier
2 siblings, 1 reply; 10+ messages in thread
From: Laurent Vivier @ 2023-01-31 12:28 UTC (permalink / raw)
To: Helge Deller, Richard Henderson, QEMU Developers
Le 12/12/2022 à 18:34, Helge Deller a écrit :
> Applications do call sendmsg() without any IOV, e.g.:
> sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=NULL, msg_iovlen=0,
> msg_control=[{cmsg_len=36, cmsg_level=SOL_ALG, cmsg_type=0x2}],
> msg_controllen=40, msg_flags=0}, MSG_MORE) = 0
> sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="The quick brown fox jumps over t"..., iov_len=183}],
> msg_iovlen=1, msg_control=[{cmsg_len=20, cmsg_level=SOL_ALG, cmsg_type=0x3}],
> msg_controllen=24, msg_flags=0}, 0) = 183
>
> The function do_sendrecvmsg_locked() is used for sndmsg() and recvmsg()
> and calls lock_iovec() to lock the IOV into memory. For the first
> sendmsg() above it returns NULL and thus wrongly skips the call the host
> sendmsg() syscall, which will break the calling application.
>
> Fix this issue by:
> - allowing sendmsg() even with empty IOV
> - skip recvmsg() if IOV is NULL
> - skip both if the return code of do_sendrecvmsg_locked() != 0, which
> indicates some failure like EFAULT on the IOV
>
> Tested with the debian "ell" package with hppa guest on x86_64 host.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> ---
> linux-user/syscall.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index a365903a3a..9e2c0a18fc 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3330,7 +3330,10 @@ static abi_long do_sendrecvmsg_locked(int fd, struct target_msghdr *msgp,
> target_vec, count, send);
> if (vec == NULL) {
> ret = -host_to_target_errno(errno);
> - goto out2;
> + /* allow sending packet without any iov, e.g. with MSG_MORE flag */
why don't you check only for count is 0?
Somehing like:
if (vec == NULL && (count || !send)) {
...
Thanks,
Laurent
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] linux-user: Allow sendmsg() without IOV
2023-01-31 12:28 ` Laurent Vivier
@ 2023-01-31 13:34 ` Helge Deller
2023-01-31 13:50 ` Helge Deller
0 siblings, 1 reply; 10+ messages in thread
From: Helge Deller @ 2023-01-31 13:34 UTC (permalink / raw)
To: Laurent Vivier, Richard Henderson, QEMU Developers
On 1/31/23 13:28, Laurent Vivier wrote:
> Le 12/12/2022 à 18:34, Helge Deller a écrit :
>> Applications do call sendmsg() without any IOV, e.g.:
>> sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=NULL, msg_iovlen=0,
>> msg_control=[{cmsg_len=36, cmsg_level=SOL_ALG, cmsg_type=0x2}],
>> msg_controllen=40, msg_flags=0}, MSG_MORE) = 0
>> sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="The quick brown fox jumps over t"..., iov_len=183}],
>> msg_iovlen=1, msg_control=[{cmsg_len=20, cmsg_level=SOL_ALG, cmsg_type=0x3}],
>> msg_controllen=24, msg_flags=0}, 0) = 183
>>
>> The function do_sendrecvmsg_locked() is used for sndmsg() and recvmsg()
>> and calls lock_iovec() to lock the IOV into memory. For the first
>> sendmsg() above it returns NULL and thus wrongly skips the call the host
>> sendmsg() syscall, which will break the calling application.
>>
>> Fix this issue by:
>> - allowing sendmsg() even with empty IOV
>> - skip recvmsg() if IOV is NULL
>> - skip both if the return code of do_sendrecvmsg_locked() != 0, which
>> indicates some failure like EFAULT on the IOV
>>
>> Tested with the debian "ell" package with hppa guest on x86_64 host.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> ---
>> linux-user/syscall.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index a365903a3a..9e2c0a18fc 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -3330,7 +3330,10 @@ static abi_long do_sendrecvmsg_locked(int fd, struct target_msghdr *msgp,
>> target_vec, count, send);
>> if (vec == NULL) {
>> ret = -host_to_target_errno(errno);
>> - goto out2;
>> + /* allow sending packet without any iov, e.g. with MSG_MORE flag */
>
> why don't you check only for count is 0?
> Somehing like:
>
> if (vec == NULL && (count || !send)) {
You mean:
if (vec == NULL && (!count || !send)) {
goto out2;
lock_iovec() sets errno=0 if count==0, but for invalid addresses it sets errno!=0,
so my current check (for which ret is the same as errno):
if (!send || ret) {
goto out2;
exits on memfaults too, while a check for just count==0 would succeed.
Helge
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] linux-user: Allow sendmsg() without IOV
2023-01-31 13:34 ` Helge Deller
@ 2023-01-31 13:50 ` Helge Deller
2023-01-31 15:17 ` Laurent Vivier
0 siblings, 1 reply; 10+ messages in thread
From: Helge Deller @ 2023-01-31 13:50 UTC (permalink / raw)
To: Laurent Vivier, Richard Henderson, QEMU Developers
On 1/31/23 14:34, Helge Deller wrote:
> On 1/31/23 13:28, Laurent Vivier wrote:
>> Le 12/12/2022 à 18:34, Helge Deller a écrit :
>>> Applications do call sendmsg() without any IOV, e.g.:
>>> sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=NULL, msg_iovlen=0,
>>> msg_control=[{cmsg_len=36, cmsg_level=SOL_ALG, cmsg_type=0x2}],
>>> msg_controllen=40, msg_flags=0}, MSG_MORE) = 0
>>> sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="The quick brown fox jumps over t"..., iov_len=183}],
>>> msg_iovlen=1, msg_control=[{cmsg_len=20, cmsg_level=SOL_ALG, cmsg_type=0x3}],
>>> msg_controllen=24, msg_flags=0}, 0) = 183
>>>
>>> The function do_sendrecvmsg_locked() is used for sndmsg() and recvmsg()
>>> and calls lock_iovec() to lock the IOV into memory. For the first
>>> sendmsg() above it returns NULL and thus wrongly skips the call the host
>>> sendmsg() syscall, which will break the calling application.
>>>
>>> Fix this issue by:
>>> - allowing sendmsg() even with empty IOV
>>> - skip recvmsg() if IOV is NULL
>>> - skip both if the return code of do_sendrecvmsg_locked() != 0, which
>>> indicates some failure like EFAULT on the IOV
>>>
>>> Tested with the debian "ell" package with hppa guest on x86_64 host.
>>>
>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>> ---
>>> linux-user/syscall.c | 9 +++++++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index a365903a3a..9e2c0a18fc 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -3330,7 +3330,10 @@ static abi_long do_sendrecvmsg_locked(int fd, struct target_msghdr *msgp,
>>> target_vec, count, send);
>>> if (vec == NULL) {
>>> ret = -host_to_target_errno(errno);
>>> - goto out2;
>>> + /* allow sending packet without any iov, e.g. with MSG_MORE flag */
>>
>> why don't you check only for count is 0?
>> Somehing like:
>>
>> if (vec == NULL && (count || !send)) {
>
> You mean:
> if (vec == NULL && (!count || !send)) {
> goto out2;
>
> lock_iovec() sets errno=0 if count==0, but for invalid addresses it sets errno!=0,
> so my current check (for which ret is the same as errno):
> if (!send || ret) {
> goto out2;
> exits on memfaults too, while a check for just count==0 would succeed.
forget the above...
Maybe just checking for count==0 would be sufficient. I don't know yet if
there are any other corner cases where replacing the check of
if (!send || ret) {
with
if (!send || count==0) {
would behave different.
Checking against errno which is reported back from lock_iovec() seemed more
logical for me. This patch has been as-is in the buildd trees now since weeks
without any problems so far at least.
Helge
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] linux-user: Allow sendmsg() without IOV
2023-01-31 13:50 ` Helge Deller
@ 2023-01-31 15:17 ` Laurent Vivier
0 siblings, 0 replies; 10+ messages in thread
From: Laurent Vivier @ 2023-01-31 15:17 UTC (permalink / raw)
To: qemu-devel
Le 31/01/2023 à 14:50, Helge Deller a écrit :
> On 1/31/23 14:34, Helge Deller wrote:
>> On 1/31/23 13:28, Laurent Vivier wrote:
>>> Le 12/12/2022 à 18:34, Helge Deller a écrit :
>>>> Applications do call sendmsg() without any IOV, e.g.:
>>>> sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=NULL, msg_iovlen=0,
>>>> msg_control=[{cmsg_len=36, cmsg_level=SOL_ALG, cmsg_type=0x2}],
>>>> msg_controllen=40, msg_flags=0}, MSG_MORE) = 0
>>>> sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="The quick brown fox jumps over
>>>> t"..., iov_len=183}],
>>>> msg_iovlen=1, msg_control=[{cmsg_len=20, cmsg_level=SOL_ALG, cmsg_type=0x3}],
>>>> msg_controllen=24, msg_flags=0}, 0) = 183
>>>>
>>>> The function do_sendrecvmsg_locked() is used for sndmsg() and recvmsg()
>>>> and calls lock_iovec() to lock the IOV into memory. For the first
>>>> sendmsg() above it returns NULL and thus wrongly skips the call the host
>>>> sendmsg() syscall, which will break the calling application.
>>>>
>>>> Fix this issue by:
>>>> - allowing sendmsg() even with empty IOV
>>>> - skip recvmsg() if IOV is NULL
>>>> - skip both if the return code of do_sendrecvmsg_locked() != 0, which
>>>> indicates some failure like EFAULT on the IOV
>>>>
>>>> Tested with the debian "ell" package with hppa guest on x86_64 host.
>>>>
>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>> ---
>>>> linux-user/syscall.c | 9 +++++++--
>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>>> index a365903a3a..9e2c0a18fc 100644
>>>> --- a/linux-user/syscall.c
>>>> +++ b/linux-user/syscall.c
>>>> @@ -3330,7 +3330,10 @@ static abi_long do_sendrecvmsg_locked(int fd, struct target_msghdr *msgp,
>>>> target_vec, count, send);
>>>> if (vec == NULL) {
>>>> ret = -host_to_target_errno(errno);
>>>> - goto out2;
>>>> + /* allow sending packet without any iov, e.g. with MSG_MORE flag */
>>>
>>> why don't you check only for count is 0?
>>> Somehing like:
>>>
>>> if (vec == NULL && (count || !send)) {
>>
>> You mean:
>> if (vec == NULL && (!count || !send)) {
>> goto out2;
>>
>> lock_iovec() sets errno=0 if count==0, but for invalid addresses it sets errno!=0,
>> so my current check (for which ret is the same as errno):
>> if (!send || ret) {
>> goto out2;
>> exits on memfaults too, while a check for just count==0 would succeed.
>
> forget the above...
> Maybe just checking for count==0 would be sufficient. I don't know yet if
> there are any other corner cases where replacing the check of
> if (!send || ret) {
> with
> if (!send || count==0) {
> would behave different.
> Checking against errno which is reported back from lock_iovec() seemed more
> logical for me. This patch has been as-is in the buildd trees now since weeks
> without any problems so far at least.
>
It seems kernel also checks for the invalid address even with count=0, so I'm going to apply this
patch as-is.
Thanks,
Laurent
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] linux-user: Allow sendmsg() without IOV
2022-12-12 17:34 ` [PATCH 2/2] linux-user: Allow sendmsg() without IOV Helge Deller
2023-01-31 12:28 ` Laurent Vivier
@ 2023-01-31 15:17 ` Laurent Vivier
2023-01-31 15:19 ` Laurent Vivier
2 siblings, 0 replies; 10+ messages in thread
From: Laurent Vivier @ 2023-01-31 15:17 UTC (permalink / raw)
To: Helge Deller, Richard Henderson, qemu-devel
Le 12/12/2022 à 18:34, Helge Deller a écrit :
> Applications do call sendmsg() without any IOV, e.g.:
> sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=NULL, msg_iovlen=0,
> msg_control=[{cmsg_len=36, cmsg_level=SOL_ALG, cmsg_type=0x2}],
> msg_controllen=40, msg_flags=0}, MSG_MORE) = 0
> sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="The quick brown fox jumps over t"..., iov_len=183}],
> msg_iovlen=1, msg_control=[{cmsg_len=20, cmsg_level=SOL_ALG, cmsg_type=0x3}],
> msg_controllen=24, msg_flags=0}, 0) = 183
>
> The function do_sendrecvmsg_locked() is used for sndmsg() and recvmsg()
> and calls lock_iovec() to lock the IOV into memory. For the first
> sendmsg() above it returns NULL and thus wrongly skips the call the host
> sendmsg() syscall, which will break the calling application.
>
> Fix this issue by:
> - allowing sendmsg() even with empty IOV
> - skip recvmsg() if IOV is NULL
> - skip both if the return code of do_sendrecvmsg_locked() != 0, which
> indicates some failure like EFAULT on the IOV
>
> Tested with the debian "ell" package with hppa guest on x86_64 host.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> ---
> linux-user/syscall.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] linux-user: Allow sendmsg() without IOV
2022-12-12 17:34 ` [PATCH 2/2] linux-user: Allow sendmsg() without IOV Helge Deller
2023-01-31 12:28 ` Laurent Vivier
2023-01-31 15:17 ` Laurent Vivier
@ 2023-01-31 15:19 ` Laurent Vivier
2 siblings, 0 replies; 10+ messages in thread
From: Laurent Vivier @ 2023-01-31 15:19 UTC (permalink / raw)
To: Helge Deller, Richard Henderson, qemu-devel
Le 12/12/2022 à 18:34, Helge Deller a écrit :
> Applications do call sendmsg() without any IOV, e.g.:
> sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=NULL, msg_iovlen=0,
> msg_control=[{cmsg_len=36, cmsg_level=SOL_ALG, cmsg_type=0x2}],
> msg_controllen=40, msg_flags=0}, MSG_MORE) = 0
> sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="The quick brown fox jumps over t"..., iov_len=183}],
> msg_iovlen=1, msg_control=[{cmsg_len=20, cmsg_level=SOL_ALG, cmsg_type=0x3}],
> msg_controllen=24, msg_flags=0}, 0) = 183
>
> The function do_sendrecvmsg_locked() is used for sndmsg() and recvmsg()
> and calls lock_iovec() to lock the IOV into memory. For the first
> sendmsg() above it returns NULL and thus wrongly skips the call the host
> sendmsg() syscall, which will break the calling application.
>
> Fix this issue by:
> - allowing sendmsg() even with empty IOV
> - skip recvmsg() if IOV is NULL
> - skip both if the return code of do_sendrecvmsg_locked() != 0, which
> indicates some failure like EFAULT on the IOV
>
> Tested with the debian "ell" package with hppa guest on x86_64 host.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> ---
> linux-user/syscall.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index a365903a3a..9e2c0a18fc 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3330,7 +3330,10 @@ static abi_long do_sendrecvmsg_locked(int fd, struct target_msghdr *msgp,
> target_vec, count, send);
> if (vec == NULL) {
> ret = -host_to_target_errno(errno);
> - goto out2;
> + /* allow sending packet without any iov, e.g. with MSG_MORE flag */
> + if (!send || ret) {
> + goto out2;
> + }
> }
> msg.msg_iovlen = count;
> msg.msg_iov = vec;
> @@ -3382,7 +3385,9 @@ static abi_long do_sendrecvmsg_locked(int fd, struct target_msghdr *msgp,
> }
>
> out:
> - unlock_iovec(vec, target_vec, count, !send);
> + if (vec) {
> + unlock_iovec(vec, target_vec, count, !send);
> + }
> out2:
> return ret;
> }
> --
> 2.38.1
>
>
Applied to my linux-user-for-8.0 branch.
Thanks,
Laurent
^ permalink raw reply [flat|nested] 10+ messages in thread