Re: [PATCH v2] linux-user: implement TARGET_SO_PEERSEC

[Laurent Vivier]

Le 12/02/2020 à 17:08, Philippe Mathieu-Daudé a écrit :
> On 2/12/20 5:03 PM, Laurent Vivier wrote:
>> Le 12/02/2020 à 16:56, Philippe Mathieu-Daudé a écrit :
>>> On 2/4/20 10:19 PM, Laurent Vivier wrote:
>>>> "The purpose of this option is to allow an application to obtain the
>>>> security credentials of a Unix stream socket peer.  It is analogous to
>>>> SO_PEERCRED (which provides authentication using standard Unix
>>>> credentials
>>>> of pid, uid and gid), and extends this concept to other security
>>>> models." -- https://lwn.net/Articles/62370/
>>>>
>>>> Until now it was passed to the kernel with an "int" argument and
>>>> fails when it was supported by the host because the parameter is
>>>> like a filename: it is always a \0-terminated string with no embedded
>>>> \0 characters, but is not guaranteed to be ASCII or UTF-8.
>>>>
>>>> I've tested the option with the following program:
>>>>
>>>>       /*
>>>>        * cc -o getpeercon getpeercon.c
>>>>        */
>>>>
>>>>       #include <stdio.h>
>>>>       #include <sys/types.h>
>>>>       #include <sys/socket.h>
>>>>       #include <netinet/in.h>
>>>>       #include <arpa/inet.h>
>>>>
>>>>       int main(void)
>>>>       {
>>>>           int fd;
>>>>           struct sockaddr_in server, addr;
>>>>           int ret;
>>>>           socklen_t len;
>>>>           char buf[256];
>>>>
>>>>           fd = socket(PF_INET, SOCK_STREAM, 0);
>>>>           if (fd == -1) {
>>>>               perror("socket");
>>>>               return 1;
>>>>           }
>>>>
>>>>           server.sin_family = AF_INET;
>>>>           inet_aton("127.0.0.1", &server.sin_addr);
>>>>           server.sin_port = htons(40390);
>>>>
>>>>           connect(fd, (struct sockaddr*)&server, sizeof(server));
>>>>
>>>>           len = sizeof(buf);
>>>>           ret = getsockopt(fd, SOL_SOCKET, SO_PEERSEC, buf, &len);
>>>>           if (ret == -1) {
>>>>               perror("getsockopt");
>>>>               return 1;
>>>>           }
>>>>           printf("%d %s\n", len, buf);
>>>>           return 0;
>>>>       }
>>>>
>>>> On host:
>>>>
>>>>     $ ./getpeercon
>>>>     33 system_u:object_r:unlabeled_t:s0
>>>>
>>>> With qemu-aarch64/bionic without the patch:
>>>>
>>>>     $ ./getpeercon
>>>>     getsockopt: Numerical result out of range
>>>>
>>>> With the patch:
>>>>
>>>>     $ ./getpeercon
>>>>     33 system_u:object_r:unlabeled_t:s0
>>>>
>>>> Bug: https://bugs.launchpad.net/qemu/+bug/1823790
>>>> Reported-by: Matthias Lüscher <address@hidden>
>>>> Tested-by: Matthias Lüscher <address@hidden>
>>>> Signed-off-by: Laurent Vivier <address@hidden>
>>>> ---
>>>>
>>>> Notes:
>>>>       v2: use correct length in unlock_user()
>>>>
>>>>    linux-user/syscall.c | 22 ++++++++++++++++++++++
>>>>    1 file changed, 22 insertions(+)
>>>>
>>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>>> index d60142f0691c..c930577686da 100644
>>>> --- a/linux-user/syscall.c
>>>> +++ b/linux-user/syscall.c
>>>> @@ -2344,6 +2344,28 @@ static abi_long do_getsockopt(int sockfd, int
>>>> level, int optname,
>>>>                }
>>>>                break;
>>>>            }
>>>> +        case TARGET_SO_PEERSEC: {
>>>> +            char *name;
>>>> +
>>>> +            if (get_user_u32(len, optlen)) {
>>>> +                return -TARGET_EFAULT;
>>>> +            }
>>>> +            if (len < 0) {
>>>> +                return -TARGET_EINVAL;
>>>> +            }
>>>> +            name = lock_user(VERIFY_WRITE, optval_addr, len, 0);
>>>> +            if (!name) {
>>>> +                return -TARGET_EFAULT;
>>>> +            }
>>>> +            lv = len;
>>>> +            ret = get_errno(getsockopt(sockfd, level, SO_PEERSEC,
>>>> +                                       name, &lv));
>>>
>>> Can we get lv > len?
>>
>> No:
>>
>> getsockopt(2)
>>
>> "For  getsockopt(), optlen is a value-result argument, initially
>> containing the size of the buffer pointed to by optval, and modified on
>> return to  indicate the  actual  size  of  the value returned."
>>
>>>
>>>> +            if (put_user_u32(lv, optlen)) {
>>>> +                ret = -TARGET_EFAULT;
>>>> +            }
>>>> +            unlock_user(name, optval_addr, lv);
>>>
>>> Maybe safer to use len instead of lv here?
>>
>> No:
>>
>> this is the length of the buffer we must copy back to the user. Kernel
>> has only modified lv length, not len.
> 
> So we can simplify the TARGET_SO_LINGER case then.

No, this case is different because lglen is sizeof(struct linger) and it
can differ from len. So lglen can be greater than len.

If you check the kernel you can see if the buffer is not big enough the
data are partially copied. This is partially done in our code because
the __put_user() can overflow the user memory but we return len to the
caller. To fix that, we should use a local target_linger to change
endianness and then copy the local copy to the user copy using len.

>>
>> linux-user/qemu.h
>>
>> /* Unlock an area of guest memory.  The first LEN bytes must be
>>     flushed back to guest memory. host_ptr = NULL is explicitly
>>     allowed and does nothing. */
>> static inline void unlock_user(void *host_ptr, abi_ulong guest_addr,
>>                                 long len)
>>
> 
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> Tested-by: Philippe Mathieu-Daudé <address@hidden>
> 

Thank you.

Laurent