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 <address@hidden>:
>
> Le 06/05/2020 à 15:21, Tomas Krcka a écrit :
> > Signed-off-by: Tomas Krcka <address@hidden>
> > ---
> >  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
>
>