netdev - Re: [GIT PULL] [IPV6] COMPAT: Fix SSM applications on 64bit kernels.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20080425.054328.51438140.yoshfuji@linux-ipv6.org>
Date:	Fri, 25 Apr 2008 05:43:28 +0900 (JST)
From:	YOSHIFUJI Hideaki / 吉藤英明 
	<yoshfuji@...ux-ipv6.org>
To:	dlstevens@...ibm.com
Cc:	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [GIT PULL] [IPV6] COMPAT: Fix SSM applications on 64bit
 kernels.

In article <OFFE40C097.9EE8B821-ON88257435.0067E798-88257435.006E0467@...ibm.com> (at Thu, 24 Apr 2008 13:01:43 -0700), David Stevens <dlstevens@...ibm.com> says:


> >  int ip6_mc_msfilter(struct sock *sk, struct group_filter *gsf)
> >  {
> >     struct in6_addr *group;
> > 
> 
>         I think this part isn't relevant to compat support, right?
> I don't see much advantage in moving this, but it should be a
> separate patch and discussion, I think.

Otherwise, we will have too much duplicate code.

> 
> > +   switch (optname) {
> > +   case MCAST_JOIN_SOURCE_GROUP:
> > +   case MCAST_LEAVE_SOURCE_GROUP:
> > +   case MCAST_BLOCK_SOURCE:
> > +   case MCAST_UNBLOCK_SOURCE:
> > +       {
> > +      struct group_source_req greqs;
> 
>         Are you sure we want this on the kernel stack?
:
> > +      struct group_req greq;
> 
>         Big for the kernel stack.

I know, and I plan to fix stack usage in the next stage.

> > +
> > +struct compat_group_filter
> > +{
> > +   __u32               gf_interface;
> > +   struct __compat_sockaddr_storage   gf_group;
> > +   __u32               gf_fmode;
> > +   __u32               gf_numsrc;
> > +   struct __compat_sockaddr_storage   gf_slist[1];
> > +};
> > +
> > +#define COMPAT_GROUP_FILTER_SIZE(numsrc) \
> > +   (sizeof(struct compat_group_filter) - sizeof(struct 
> __compat_sockaddr_storage) \
> > +   + (numsrc) * sizeof(struct __compat_sockaddr_storage))
> >  #endif
> >  #endif
> 
>         Better to compute this just once, in which case no macro needed.
> Suggest a variable and do
> "len = GROUP_FILTER_SIZE(numsrc) + sizeof(struct compat_group_filter) - 
> sizeof(struct group_filter);"
> Also, why put these in an include file when there is only one use?

It is much clearer to mirror original logic.

> > +      optlen = GROUP_FILTER_SIZE(gf_numsrc);
> > +      gsf = kmalloc(optlen, GFP_KERNEL);
> > +      if (!gsf) {
> > +         retv = -ENOBUFS;
> > +         break;
> > +      }
> > +      retv = -EFAULT;
> > +      if (__get_user(gsf->gf_interface, &ugsf->gf_interface) ||
> > +          __copy_from_user(&gsf->gf_group, &ugsf->gf_group, 
> sizeof(gsf->gf_group)) ||
> > +          __get_user(gsf->gf_fmode, &ugsf->gf_fmode) ||
> > +          access_ok(VERIFY_READ, ugsf->gf_slist,
> > +               sizeof(struct __compat_sockaddr_storage) * gf_numsrc))
> > +         goto kfree_out;
> 
>         Does access_ok() handle (valid) case numsrc==0 correctly?

Hmm, not sure...

> > +      gsf->gf_numsrc = gf_numsrc;
> > +
> > +      for (i = 0; i < gf_numsrc; i++) {
> > +         if (__copy_from_user(&gsf->gf_slist[i],
> > +                    &ugsf->gf_slist[i],
> > +                    sizeof(gsf->gf_slist[i])))
> > +            goto kfree_out;
> > +      }
> 
>         This is adding a lot of separate copies that I believe
> are unnecessary. sockaddr_storage is the same size, so you
> can copy optlen-GROUP_FILTER_SIZE(0) all at once.

Okay...

> > -int ip6_mc_msfget(struct sock *sk, struct group_filter *gsf,
> > -   struct group_filter __user *optval, int __user *optlen)
> > +int __ip6_mc_msf_lookup(struct sock *sk,
:
> > +      goto out;
> > +   }
> 
>         Looks unrelated to compat support...? As what follows in
> this function.

For killing duplicates between native / compat code.

> > +
> > +      copycount = count < gf_numsrc ? count : gf_numsrc;
> > +
> > +      if (put_user(COMPAT_GROUP_FILTER_SIZE(copycount), optlen) ||
> > +          access_ok(VERIFY_WRITE, optval, 
> COMPAT_GROUP_FILTER_SIZE(copycount)) ||
> 
>         numsrc == 0 case works?

???

> > +         ipv6_addr_copy(&sin6.sin6_addr, &psl->sl_addr[i]);
> > +
> > +         if (__copy_to_user(usin6, &sin6, sizeof(sin6)) ||
> > +             __clear_user(usin6 + 1, sizeof(optval->gf_slist[i]) - 
> sizeof(*usin6)))
> 
> why explicitly clear the rest of it?

We mirror original behavior, which clears rest of sockaddr_in6{}.

--yoshfuji
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html