lkml.org 
[lkml]   [2010]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [PATCH] wireless: fix 64K kernel heap content leak via ioctl
    From
    Date
    On Mon, 2010-08-30 at 10:47 +0200, Johannes Berg wrote:
    > On Fri, 2010-08-27 at 14:22 -0700, Jean Tourrilhes wrote:
    >
    > > Would you mind validating the following patch ? I've just
    > > verified that it compiles and I believe it does what you are asking in
    > > a much more predictable way.
    > > Regards,
    >
    >
    > > @@ -800,9 +800,12 @@ static int ioctl_standard_iw_point(struc
    > > goto out;
    > > }
    > >
    > > - if (copy_to_user(iwp->pointer, extra,
    > > - iwp->length *
    > > - descr->token_size)) {
    > > + /* Verify how much we should return. Some driver
    > > + * may abuse iwp->length... */
    > > + if((iwp->length * descr->token_size) < extra_size)
    > > + extra_size = iwp->length * descr->token_size;
    > > +
    > > + if (copy_to_user(iwp->pointer, extra, extra_size)) {
    > > err = -EFAULT;
    > > goto out;
    >
    > Based on the code _before_ this hunk, I believe this patch to be wrong
    > (the goto out matches):
    >
    > /* If we have something to return to the user */
    > if (!err && IW_IS_GET(cmd)) {
    > /* Check if there is enough buffer up there */
    > if (user_length < iwp->length) {
    > err = -E2BIG;
    > goto out;
    > }
    >
    > Thus, apparently drivers were intended to be allowed to return more
    > information than userspace had allocated space for (which also matches
    > the initial extra_size calculation in this function), so your comment is
    > wrong, and your check is also wrong because you actually put the burden
    > on the driver, contrary to the apparent intention of this code.
    >
    > I believe the below patch is a much better fix as it allows the -E2BIG
    > code path to be invoked which is more informative to users than
    > truncated information (which, in your code, may even be truncated in the
    > middle of a token!!)

    err, forgot quilt refresh, here's the right patch:

    ---
    net/wireless/wext-core.c | 23 ++++++++++-------------
    1 file changed, 10 insertions(+), 13 deletions(-)

    --- wireless-testing.orig/net/wireless/wext-core.c 2010-08-30 10:35:33.000000000 +0200
    +++ wireless-testing/net/wireless/wext-core.c 2010-08-30 10:57:38.000000000 +0200
    @@ -738,26 +738,23 @@ static int ioctl_standard_iw_point(struc
    /* Save user space buffer size for checking */
    user_length = iwp->length;

    - /* Don't check if user_length > max to allow forward
    - * compatibility. The test user_length < min is
    - * implied by the test at the end.
    - */
    -
    /* Support for very large requests */
    - if ((descr->flags & IW_DESCR_FLAG_NOMAX) &&
    - (user_length > descr->max_tokens)) {
    + if (descr->flags & IW_DESCR_FLAG_NOMAX) {
    /* Allow userspace to GET more than max so
    * we can support any size GET requests.
    - * There is still a limit : -ENOMEM.
    - */
    - extra_size = user_length * descr->token_size;
    -
    - /* Note : user_length is originally a __u16,
    + * There is still a limit: 64k records of
    + * token_size (since a u16 is used).
    + *
    + * Note: user_length is originally a u16,
    * and token_size is controlled by us,
    * so extra_size won't get negative and
    * won't overflow...
    */
    - }
    + if (user_length > descr->max_tokens)
    + extra_size = user_length * descr->token_size;
    + } else
    + user_length = min_t(int, user_length,
    + descr->max_tokens);
    }

    /* kzalloc() ensures NULL-termination for essid_compat. */



    \
     
     \ /
      Last update: 2010-08-30 11:01    [W:2.557 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site