[lkml]   [2009]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] [0/11] SYSCTL: Use RCU to avoid races with string sysctls
    Andi Kleen <> writes:

    > On Sun, Dec 20, 2009 at 05:59:59PM -0800, Eric W. Biederman wrote:
    >> Andi Kleen <> writes:
    >> > With BKL-less sysctls most of the writable string sysctls are racy. There
    >> > is no locking on the reader side, so a reader could see an inconsistent
    >> > string or worse miss the terminating null and walk of beyond it.
    >> The walk will only extend up to the maximum length of the string.
    >> So the worst case really is inconsistent data.
    > It could still miss the 0 byte and walk out, can't it?

    Looking again. Yes it appears there is a small vulnerability there.

    The code does:

    len = strlen(data);
    if (len > maxlen)
    len = maxlen;

    So we should be calling:
    len = strnlen(data, maxlen);

    At which point we won't be able to walk out.

    The write side appears to be in need of strnlen_user
    as well, so it does not walk all of user space looking
    for null byte.

    >> This is an unfortunate corner case. This is not a regression as this
    >> has been the way things have worked for years. So probably 2.6.34
    >> material.
    > The one that's a clear regression is the core pattern one, that
    > was protected before by the BKL. A lot of others were always
    > broken yes.

    Nope. The core pattern just thought it was protected by BKL. I did
    not change the /proc/sys code path to remove the BKL. I don't know
    if we ever took the BKL on the /proc/sys codepath.

    I remember looking at the core pattern earlier and my memory is that
    sysctl is new enough that core pattern was not protected by the BKL on the
    /proc/sys path when it was introduced.

    There was a lot of confusing code in the sys_sysctl code path (which
    grabbed the BKL) so I expect people thought they were safe due to the
    BKL when they were not.

    So we have sysctl have locking problems, not new sysctl regressions.

    >> > This patch kit adds a new "rcu string" variant to avoid these
    >> > problems and convers the racy users. One the writer side the strings are
    >> > always copied to new memory and the readers use rcu_read_lock()
    >> > to get a stable view. For readers who access the string over
    >> > sleeps the reader copies the string.
    >> I will have to look more after the holidays. This rcu_string looks like
    >> it introduces allocations on paths that did not use them before, which
    >> has me wondering a bit.
    > On the reader side about all of them allocated before, e.g. for
    > call_usermodehelper.

    That sounds like less of an issue.

    > If the strings were made a bit smaller this could be also
    > put on the stack, but I didn't dare for 256 bytes.

    Hmm. rcu wise that sounds wrong, but I haven't looked into your
    cool new data structure yet.


     \ /
      Last update: 2009-12-21 03:33    [W:0.025 / U:0.732 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site