netdev - Re: [PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options

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]

Date:   Sat, 23 Jun 2018 21:16:18 +0900 (KST)
From:   David Miller <davem@...emloft.net>
To:     pmoore@...hat.com
Cc:     netdev@...r.kernel.org, selinux@...ho.nsa.gov,
        linux-security-module@...r.kernel.org
Subject: Re: [PATCH] ipv6: avoid copy_from_user() via
 ipv6_renew_options_kern()

From: Paul Moore <pmoore@...hat.com>
Date: Fri, 22 Jun 2018 17:18:20 -0400

> From: Paul Moore <paul@...l-moore.com>
> 
> The ipv6_renew_options_kern() function eventually called into
> copy_from_user(), despite it not using any userspace buffers, which
> was problematic as that ended up calling access_ok() which emited
> a warning on x86 (and likely other arches as well).
> 
>   ipv6_renew_options_kern()
>     ipv6_renew_options()
>       ipv6_renew_option()
>         copy_from_user()
>           _copy_from_user()
>             access_ok()
> 
> The access_ok() check inside _copy_from_user() is obviously the right
> thing to do which means that calling copy_from_user() via
> ipv6_renew_options_kern() is obviously the wrong thing to do.

Ok, I re-read the code around here.

access_ok() is not warning because we are calling copy_from_user()
with a kernel pointer.  The set_ds(KERNEL_DS) adjusts the
user_addr_max() setting, and thus that check passes.

The problem is that we are invoking this from an interrupt, and this
triggers the WARN_ON_IN_IRQ() in access_ok().

Although I think that WARN_ON_IN_IRQ() is completely unnecessary when
KERNEL_DS is set, the situation that really causes this problem is not
at all clear from your commit message.

I guess that for now your fix is fine, but I want you to please adjust
the commit message.

Provide the _full_ annotated kernel backtrace from the warning that
triggers, because this will show the reader that we are in an
interrupt.  And explain that being in the interrupt is strictly what
causes this to warn, not that we are using kernel pointers.  The
latter is %100 valid when set_fs(KERNEL_DS) is performed.

Thank you.