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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 2/12/20 5:43 PM, Laurent Vivier wrote:
> 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 <lueschem@gmail.com>
>>>>> Tested-by: Matthias Lüscher <lueschem@gmail.com>
>>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>>>> ---
>>>>>
>>>>> 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.

Ah OK I understand now, thanks for the explanation.

> 
>>>
>>> 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é <philmd@redhat.com>
>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
> 
> Thank you.
> 
> Laurent
>