[PATCH 1/2] linux-user: add do_setsockopt SOL_CAN_RAW CAN_RAW_FILTER support

[PATCH 1/2] linux-user: add do_setsockopt SOL_CAN_RAW CAN_RAW_FILTER support

[Tomas Krcka]

Signed-off-by: Tomas Krcka <tomas.krcka@gmail.com>
---
 linux-user/syscall.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 05f03919ff..88d4c85b70 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -56,6 +56,7 @@
 #include <linux/wireless.h>
 #include <linux/icmp.h>
 #include <linux/icmpv6.h>
+#include <linux/can/raw.h>
 #include <linux/errqueue.h>
 #include <linux/random.h>
 #ifdef CONFIG_TIMERFD
@@ -2111,6 +2112,39 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,
             goto unimplemented;
         }
         break;
+    case SOL_CAN_RAW:
+        switch (optname) {
+        case CAN_RAW_FILTER:
+        {
+            if (optlen % sizeof(struct can_filter) != 0) {
+                return -TARGET_EINVAL;
+            }
+
+            struct can_filter  *can_filters = NULL;
+            if (optlen != 0) {
+                can_filters = g_new0(struct can_filter, optlen);
+
+                if (!can_filters) {
+                    return -TARGET_ENOMEM;
+                }
+                if (copy_from_user(can_filters, optval_addr, optlen)) {
+                    g_free(can_filters);
+                    return -TARGET_EFAULT;
+                }
+                for (val = 0; val < optlen / sizeof(struct can_filter); val++) {
+                    can_filters[val].can_id = tswap32(can_filters[val].can_id);
+                    can_filters[val].can_mask = tswap32(can_filters[val].can_mask);
+                }
+            }
+            ret = get_errno(setsockopt(sockfd, level, optname,
+                                        can_filters, optlen));
+            g_free(can_filters);
+            break;
+        }
+        default:
+            goto unimplemented;
+        }
+        break;
     case SOL_RAW:
         switch (optname) {
         case ICMP_FILTER:
-- 
2.17.1

[PATCH 2/2] linux-user: add do_setsockopt CAN_RAW_FD_FRAMES support

[Tomas Krcka]

Signed-off-by: Tomas Krcka <tomas.krcka@gmail.com>
---
 linux-user/syscall.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 88d4c85b70..f751ed8b37 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2141,6 +2141,19 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,
             g_free(can_filters);
             break;
         }
+        case CAN_RAW_FD_FRAMES:
+        {
+            val = 0;
+            if (optlen < sizeof(uint32_t)) {
+                return -TARGET_EINVAL;
+            }
+            if (get_user_u32(val, optval_addr)) {
+                return -TARGET_EFAULT;
+            }
+            ret = get_errno(setsockopt(sockfd, level, optname,
+                                    &val, sizeof(val)));
+            break;
+        }
         default:
             goto unimplemented;
         }
-- 
2.17.1

Re: [PATCH 1/2] linux-user: add do_setsockopt SOL_CAN_RAW CAN_RAW_FILTER support

[Laurent Vivier]

Le 06/05/2020 à 15:21, Tomas Krcka a écrit :
> Signed-off-by: Tomas Krcka <tomas.krcka@gmail.com>
> ---
>  linux-user/syscall.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 05f03919ff..88d4c85b70 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -56,6 +56,7 @@
>  #include <linux/wireless.h>
>  #include <linux/icmp.h>
>  #include <linux/icmpv6.h>
> +#include <linux/can/raw.h>
>  #include <linux/errqueue.h>
>  #include <linux/random.h>
>  #ifdef CONFIG_TIMERFD
> @@ -2111,6 +2112,39 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,
>              goto unimplemented;
>          }
>          break;
> +    case SOL_CAN_RAW:
> +        switch (optname) {
> +        case CAN_RAW_FILTER:
> +        {
> +            if (optlen % sizeof(struct can_filter) != 0) {
> +                return -TARGET_EINVAL;
> +            }
> +
> +            struct can_filter  *can_filters = NULL;

Move the declaration to the top of the block.

> +            if (optlen != 0) {

If you check, like in kernel, "optlen > CAN_RAW_FILTER_MAX *
sizeof(struct can_filter)", you can exit here (and no need to set
can_filters to NULL).

> +                can_filters = g_new0(struct can_filter, optlen);
> +
> +                if (!can_filters) {

no need to check the result, g_new0() aborts in case of problem.

> +                    return -TARGET_ENOMEM;
> +                }
> +                if (copy_from_user(can_filters, optval_addr, optlen)) {
> +                    g_free(can_filters);
> +                    return -TARGET_EFAULT;
> +                }

It would be cleaner not to use the copy_from_user() as we need to
byte-swap all the fields in the loop below (I know it's done like that
in SOL_ICMPV6...)

> +                for (val = 0; val < optlen / sizeof(struct can_filter); val++) {
> +                    can_filters[val].can_id = tswap32(can_filters[val].can_id);
> +                    can_filters[val].can_mask = tswap32(can_filters[val].can_mask);
> +                }

So, something like:

target_can_filters = lock_user(VERIFY_READ, optval_addr, optlen, 1);
for (val = 0; val < optlen / sizeof(struct can_filter); val++) {
    __get_user(can_filters[val].can_id, \
               &target_can_filters[val].can_id);
    __get_user(can_filters[val].can_mask, \
               &target_can_filters[val].can_mask);
}
unlock_user(target_can_filters);

> +            }
> +            ret = get_errno(setsockopt(sockfd, level, optname,
> +                                        can_filters, optlen));
> +            g_free(can_filters);
> +            break;
> +        }
> +        default:
> +            goto unimplemented;
> +        }
> +        break;
>      case SOL_RAW:
>          switch (optname) {
>          case ICMP_FILTER:
> 

Could you also update getsockopt()?

Thanks,
Laurent

Re: [PATCH 2/2] linux-user: add do_setsockopt CAN_RAW_FD_FRAMES support

[Laurent Vivier]

Le 06/05/2020 à 15:21, Tomas Krcka a écrit :
> Signed-off-by: Tomas Krcka <tomas.krcka@gmail.com>
> ---
>  linux-user/syscall.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 88d4c85b70..f751ed8b37 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -2141,6 +2141,19 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,
>              g_free(can_filters);
>              break;
>          }
> +        case CAN_RAW_FD_FRAMES:
> +        {
> +            val = 0;

You don't need to set val to 0.

> +            if (optlen < sizeof(uint32_t)) {

kernel checks for the exact size and fails otherwise.

> +                return -TARGET_EINVAL;
> +            }
> +            if (get_user_u32(val, optval_addr)) {
> +                return -TARGET_EFAULT;
> +            }
> +            ret = get_errno(setsockopt(sockfd, level, optname,
> +                                    &val, sizeof(val)));
> +            break;
> +        }
>          default:
>              goto unimplemented;
>          }
> 

Could you implement getsockopt() too?

Thanks,
Laurent

Re: [PATCH 1/2] linux-user: add do_setsockopt SOL_CAN_RAW CAN_RAW_FILTER support

[Tomas Krcka]

Am Di., 12. Mai 2020 um 22:09 Uhr schrieb Laurent Vivier <laurent@vivier.eu>:
>
> Le 06/05/2020 à 15:21, Tomas Krcka a écrit :
> > Signed-off-by: Tomas Krcka <tomas.krcka@gmail.com>
> > ---
> >  linux-user/syscall.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 05f03919ff..88d4c85b70 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -56,6 +56,7 @@
> >  #include <linux/wireless.h>
> >  #include <linux/icmp.h>
> >  #include <linux/icmpv6.h>
> > +#include <linux/can/raw.h>
> >  #include <linux/errqueue.h>
> >  #include <linux/random.h>
> >  #ifdef CONFIG_TIMERFD
> > @@ -2111,6 +2112,39 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,
> >              goto unimplemented;
> >          }
> >          break;
> > +    case SOL_CAN_RAW:
> > +        switch (optname) {
> > +        case CAN_RAW_FILTER:
> > +        {
> > +            if (optlen % sizeof(struct can_filter) != 0) {
> > +                return -TARGET_EINVAL;
> > +            }
> > +
> > +            struct can_filter  *can_filters = NULL;
>
> Move the declaration to the top of the block.
>
> > +            if (optlen != 0) {
>
> If you check, like in kernel, "optlen > CAN_RAW_FILTER_MAX *
> sizeof(struct can_filter)", you can exit here (and no need to set
> can_filters to NULL).
>

The optlen can be 0 and then the can_filter shall be NULL, based on
the socketcan
documentation.
And an additional question, shall I check if optlen is 1 and then use
non-dynamic allocated
filters, as it's done in kernel?

> > +                can_filters = g_new0(struct can_filter, optlen);
> > +
> > +                if (!can_filters) {
>
> no need to check the result, g_new0() aborts in case of problem.
>
> > +                    return -TARGET_ENOMEM;
> > +                }
> > +                if (copy_from_user(can_filters, optval_addr, optlen)) {
> > +                    g_free(can_filters);
> > +                    return -TARGET_EFAULT;
> > +                }
>
> It would be cleaner not to use the copy_from_user() as we need to
> byte-swap all the fields in the loop below (I know it's done like that
> in SOL_ICMPV6...)
>
> > +                for (val = 0; val < optlen / sizeof(struct can_filter); val++) {
> > +                    can_filters[val].can_id = tswap32(can_filters[val].can_id);
> > +                    can_filters[val].can_mask = tswap32(can_filters[val].can_mask);
> > +                }
>
> So, something like:
>
> target_can_filters = lock_user(VERIFY_READ, optval_addr, optlen, 1);
> for (val = 0; val < optlen / sizeof(struct can_filter); val++) {
>     __get_user(can_filters[val].can_id, \
>                &target_can_filters[val].can_id);
>     __get_user(can_filters[val].can_mask, \
>                &target_can_filters[val].can_mask);
> }
> unlock_user(target_can_filters);
>
> > +            }
> > +            ret = get_errno(setsockopt(sockfd, level, optname,
> > +                                        can_filters, optlen));
> > +            g_free(can_filters);
> > +            break;
> > +        }
> > +        default:
> > +            goto unimplemented;
> > +        }
> > +        break;
> >      case SOL_RAW:
> >          switch (optname) {
> >          case ICMP_FILTER:
> >
>
> Could you also update getsockopt()?
Yes, I can.
>
> Thanks,
> Laurent
>
>

Re: [PATCH 1/2] linux-user: add do_setsockopt SOL_CAN_RAW CAN_RAW_FILTER support

[Laurent Vivier]

Le 12/05/2020 à 23:05, Tomas Krcka a écrit :
> Am Di., 12. Mai 2020 um 22:09 Uhr schrieb Laurent Vivier <laurent@vivier.eu>:
>>
>> Le 06/05/2020 à 15:21, Tomas Krcka a écrit :
>>> Signed-off-by: Tomas Krcka <tomas.krcka@gmail.com>
>>> ---
>>>  linux-user/syscall.c | 34 ++++++++++++++++++++++++++++++++++
>>>  1 file changed, 34 insertions(+)
>>>
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index 05f03919ff..88d4c85b70 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -56,6 +56,7 @@
>>>  #include <linux/wireless.h>
>>>  #include <linux/icmp.h>
>>>  #include <linux/icmpv6.h>
>>> +#include <linux/can/raw.h>
>>>  #include <linux/errqueue.h>
>>>  #include <linux/random.h>
>>>  #ifdef CONFIG_TIMERFD
>>> @@ -2111,6 +2112,39 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,
>>>              goto unimplemented;
>>>          }
>>>          break;
>>> +    case SOL_CAN_RAW:
>>> +        switch (optname) {
>>> +        case CAN_RAW_FILTER:
>>> +        {
>>> +            if (optlen % sizeof(struct can_filter) != 0) {
>>> +                return -TARGET_EINVAL;
>>> +            }
>>> +
>>> +            struct can_filter  *can_filters = NULL;
>>
>> Move the declaration to the top of the block.
>>
>>> +            if (optlen != 0) {
>>
>> If you check, like in kernel, "optlen > CAN_RAW_FILTER_MAX *
>> sizeof(struct can_filter)", you can exit here (and no need to set
>> can_filters to NULL).
>>
> 
> The optlen can be 0 and then the can_filter shall be NULL, based on
> the socketcan
> documentation.

Yes, you're right I misread the kernel code.

But check optlen is lesser than "CAN_RAW_FILTER_MAX * sizeof(struct
can_filter)" to avoir too big g_new0() allocation.

And in fact "g_new0()" is wrong in your code: optlen is the byte size,
not the number of entries. You should use g_malloc0(optlen).

> And an additional question, shall I check if optlen is 1 and then use
> non-dynamic allocated
> filters, as it's done in kernel?

No, keep the code as simple as possible.

Thanks,
Laurent