[PATCH 0/9] strict user copy checks on x86_64

[PATCH 0/9] strict user copy checks on x86_64

[Stephen Boyd]

It turns out that strict user copy checks (also known as
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS) isn't actually implemented
on x86_64 and thus we aren't catching potential security holes
at compile time.

This series adds support for strict user copy checks on x86_64
and silences all the benign warnings in the x86_64 allyesconfig.

The final patch consolidates the config option as its duplicated
across mutliple arches. I don't know what tree this series should
go through so I tried to send the individual driver patches to the
respective maintainers.

Stephen Boyd (9):
  iwlegacy: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning
  iwlwifi: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning
  [SCSI] lpfc: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning
  debugfs: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning
  kprobes: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning
  Bluetooth: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning
  ASoC: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning
  x86: Implement strict user copy checks for x86_64
  Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

 arch/parisc/Kconfig                         |    1 +
 arch/parisc/Kconfig.debug                   |   14 --------------
 arch/s390/Kconfig                           |    1 +
 arch/s390/Kconfig.debug                     |   14 --------------
 arch/s390/lib/Makefile                      |    1 -
 arch/s390/lib/usercopy.c                    |    8 --------
 arch/sparc/lib/Makefile                     |    1 -
 arch/sparc/lib/usercopy.c                   |    8 --------
 arch/tile/Kconfig                           |    8 +-------
 arch/tile/include/asm/uaccess.h             |    7 ++++++-
 arch/tile/lib/uaccess.c                     |    8 --------
 arch/x86/Kconfig                            |    1 +
 arch/x86/Kconfig.debug                      |   14 --------------
 arch/x86/include/asm/uaccess_64.h           |   12 +++++++++---
 arch/x86/lib/usercopy_32.c                  |    6 ------
 drivers/net/wireless/iwlegacy/iwl-4965-rs.c |    2 +-
 drivers/net/wireless/iwlwifi/iwl-agn-rs.c   |    2 +-
 drivers/scsi/lpfc/lpfc_debugfs.c            |    3 ++-
 fs/debugfs/file.c                           |    2 +-
 kernel/kprobes.c                            |    2 +-
 lib/Kconfig.debug                           |   18 ++++++++++++++++++
 lib/Makefile                                |    1 +
 lib/usercopy.c                              |    8 ++++++++
 net/bluetooth/rfcomm/sock.c                 |    3 ++-
 sound/soc/soc-core.c                        |    2 +-
 25 files changed, 55 insertions(+), 92 deletions(-)
 delete mode 100644 arch/s390/lib/usercopy.c
 delete mode 100644 arch/sparc/lib/usercopy.c
 create mode 100644 lib/usercopy.c

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 1/9] iwlegacy: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning

[Stephen Boyd]

Enabling DEBUG_STRICT_USER_COPY_CHECKS causes the following
warning:

In file included from arch/x86/include/asm/uaccess.h:573,
                 from include/net/checksum.h:25,
                 from include/linux/skbuff.h:28,
                 from drivers/net/wireless/iwlegacy/iwl-4965-rs.c:28:
In function 'copy_from_user',
    inlined from 'iwl4965_rs_sta_dbgfs_scale_table_write' at
    drivers/net/wireless/iwlegacy/iwl-4965-rs.c:2616:
arch/x86/include/asm/uaccess_64.h:65:
warning: call to 'copy_from_user_overflow' declared with
attribute warning: copy_from_user() buffer size is not provably
correct

presumably due to buf_size being signed causing GCC to fail to
see that buf_size can't become negative.

Cc: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/net/wireless/iwlegacy/iwl-4965-rs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/iwlegacy/iwl-4965-rs.c b/drivers/net/wireless/iwlegacy/iwl-4965-rs.c
index 31ac672..cc4751c 100644
--- a/drivers/net/wireless/iwlegacy/iwl-4965-rs.c
+++ b/drivers/net/wireless/iwlegacy/iwl-4965-rs.c
@@ -2604,7 +2604,7 @@ static ssize_t iwl4965_rs_sta_dbgfs_scale_table_write(struct file *file,
 	struct iwl_lq_sta *lq_sta = file->private_data;
 	struct iwl_priv *priv;
 	char buf[64];
-	int buf_size;
+	size_t buf_size;
 	u32 parsed_rate;
 	struct iwl_station_priv *sta_priv =
 		container_of(lq_sta, struct iwl_station_priv, lq_sta);
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/9] iwlwifi: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning

[Stephen Boyd]

Enabling DEBUG_STRICT_USER_COPY_CHECKS causes the following
warning:

In file included from arch/x86/include/asm/uaccess.h:573,
                 from include/net/checksum.h:25,
                 from include/linux/skbuff.h:28,
                 from drivers/net/wireless/iwlwifi/iwl-agn-rs.c:28:
In function 'copy_from_user',
    inlined from 'rs_sta_dbgfs_scale_table_write' at
    drivers/net/wireless/iwlwifi/iwl-agn-rs.c:3099:
arch/x86/include/asm/uaccess_64.h:65:
warning: call to 'copy_from_user_overflow' declared with
attribute warning: copy_from_user() buffer size is not provably
correct

presumably due to buf_size being signed causing GCC to fail to
see that buf_size can't become negative.

Cc: Wey-Yi Guy <wey-yi.w.guy@intel.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/net/wireless/iwlwifi/iwl-agn-rs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-rs.c b/drivers/net/wireless/iwlwifi/iwl-agn-rs.c
index d03b473..abcc9f2 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn-rs.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn-rs.c
@@ -3087,7 +3087,7 @@ static ssize_t rs_sta_dbgfs_scale_table_write(struct file *file,
 	struct iwl_lq_sta *lq_sta = file->private_data;
 	struct iwl_priv *priv;
 	char buf[64];
-	int buf_size;
+	size_t buf_size;
 	u32 parsed_rate;
 	struct iwl_station_priv *sta_priv =
 		container_of(lq_sta, struct iwl_station_priv, lq_sta);
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 6/9] Bluetooth: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning

[Stephen Boyd]

Enabling DEBUG_STRICT_USER_COPY_CHECKS causes the following
warning:

In function 'copy_from_user',
    inlined from 'rfcomm_sock_setsockopt' at
    net/bluetooth/rfcomm/sock.c:705:
arch/x86/include/asm/uaccess_64.h:65:
warning: call to 'copy_from_user_overflow' declared with
attribute warning: copy_from_user() buffer size is not provably
correct

presumably due to buf_size being signed causing GCC to fail to
see that buf_size can't become negative.

Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Gustavo F. Padovan <padovan@profusion.mobi>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 net/bluetooth/rfcomm/sock.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 66cc1f0..0698b37 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -679,7 +679,8 @@ static int rfcomm_sock_setsockopt(struct socket *sock, int level, int optname, c
 {
 	struct sock *sk = sock->sk;
 	struct bt_security sec;
-	int len, err = 0;
+	int err = 0;
+	size_t len;
 	u32 opt;
 
 	BT_DBG("sk %p", sk);
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 1/9] iwlegacy: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning

[wwguy]

On Thu, 2011-05-12 at 16:50 -0700, Stephen Boyd wrote:
> Enabling DEBUG_STRICT_USER_COPY_CHECKS causes the following
> warning:
> 
> In file included from arch/x86/include/asm/uaccess.h:573,
>                  from include/net/checksum.h:25,
>                  from include/linux/skbuff.h:28,
>                  from drivers/net/wireless/iwlegacy/iwl-4965-rs.c:28:
> In function 'copy_from_user',
>     inlined from 'iwl4965_rs_sta_dbgfs_scale_table_write' at
>     drivers/net/wireless/iwlegacy/iwl-4965-rs.c:2616:
> arch/x86/include/asm/uaccess_64.h:65:
> warning: call to 'copy_from_user_overflow' declared with
> attribute warning: copy_from_user() buffer size is not provably
> correct
> 
> presumably due to buf_size being signed causing GCC to fail to
> see that buf_size can't become negative.
> 
> Cc: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  drivers/net/wireless/iwlegacy/iwl-4965-rs.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
Stanislaw Gruszka <sgruszka@redhat.com> is the maintainer for iwlegacy

Thanks
Wey

Re: [PATCH 2/9] iwlwifi: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning

[wwguy]

On Thu, 2011-05-12 at 16:50 -0700, Stephen Boyd wrote:
> Enabling DEBUG_STRICT_USER_COPY_CHECKS causes the following
> warning:
> 
> In file included from arch/x86/include/asm/uaccess.h:573,
>                  from include/net/checksum.h:25,
>                  from include/linux/skbuff.h:28,
>                  from drivers/net/wireless/iwlwifi/iwl-agn-rs.c:28:
> In function 'copy_from_user',
>     inlined from 'rs_sta_dbgfs_scale_table_write' at
>     drivers/net/wireless/iwlwifi/iwl-agn-rs.c:3099:
> arch/x86/include/asm/uaccess_64.h:65:
> warning: call to 'copy_from_user_overflow' declared with
> attribute warning: copy_from_user() buffer size is not provably
> correct
> 
> presumably due to buf_size being signed causing GCC to fail to
> see that buf_size can't become negative.
> 
> Cc: Wey-Yi Guy <wey-yi.w.guy@intel.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>
> ---
>  drivers/net/wireless/iwlwifi/iwl-agn-rs.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
Thank you

Wey

Re: [PATCH 1/9] iwlegacy: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning

[Stanislaw Gruszka]

On Thu, May 12, 2011 at 04:50:04PM -0700, Stephen Boyd wrote:
> Enabling DEBUG_STRICT_USER_COPY_CHECKS causes the following
> warning:
> 
> In file included from arch/x86/include/asm/uaccess.h:573,
>                  from include/net/checksum.h:25,
>                  from include/linux/skbuff.h:28,
>                  from drivers/net/wireless/iwlegacy/iwl-4965-rs.c:28:
> In function 'copy_from_user',
>     inlined from 'iwl4965_rs_sta_dbgfs_scale_table_write' at
>     drivers/net/wireless/iwlegacy/iwl-4965-rs.c:2616:
> arch/x86/include/asm/uaccess_64.h:65:
> warning: call to 'copy_from_user_overflow' declared with
> attribute warning: copy_from_user() buffer size is not provably
> correct
> 
> presumably due to buf_size being signed causing GCC to fail to
> see that buf_size can't become negative.
> 
> Cc: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  drivers/net/wireless/iwlegacy/iwl-4965-rs.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/wireless/iwlegacy/iwl-4965-rs.c b/drivers/net/wireless/iwlegacy/iwl-4965-rs.c
> index 31ac672..cc4751c 100644
> --- a/drivers/net/wireless/iwlegacy/iwl-4965-rs.c
> +++ b/drivers/net/wireless/iwlegacy/iwl-4965-rs.c
> @@ -2604,7 +2604,7 @@ static ssize_t iwl4965_rs_sta_dbgfs_scale_table_write(struct file *file,
>  	struct iwl_lq_sta *lq_sta = file->private_data;
>  	struct iwl_priv *priv;
>  	char buf[64];
> -	int buf_size;
> +	size_t buf_size;

ACK, but we have more of that, perhaps you would like to fix them all.
If you don't want to, I'll do it. Note this can be easily done by:

sed -i 's/int buf_size;/size_t buf_size;/g' drivers/net/wireless/iwlegacy/*.[ch]

Stanislaw

Re: [PATCH 6/9] Bluetooth: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning

[Gustavo F. Padovan]

Hi Stephen,

* Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> [2011-05-12 16:50:09 -0700]:

> Enabling DEBUG_STRICT_USER_COPY_CHECKS causes the following
> warning:
> 
> In function 'copy_from_user',
>     inlined from 'rfcomm_sock_setsockopt' at
>     net/bluetooth/rfcomm/sock.c:705:
> arch/x86/include/asm/uaccess_64.h:65:
> warning: call to 'copy_from_user_overflow' declared with
> attribute warning: copy_from_user() buffer size is not provably
> correct
> 
> presumably due to buf_size being signed causing GCC to fail to
> see that buf_size can't become negative.
> 
> Cc: Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>
> Cc: Gustavo F. Padovan <padovan-Y3ZbgMPKUGA34EUeqzHoZw@public.gmane.org>
> Signed-off-by: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>  net/bluetooth/rfcomm/sock.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)

Applied, thanks.

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH 0/9] strict user copy checks on x86_64

[Stephen Boyd]

Hi Andrew,

(I don't know who to pick on sorry)

On 05/12/2011 04:50 PM, Stephen Boyd wrote:
> It turns out that strict user copy checks (also known as
> CONFIG_DEBUG_STRICT_USER_COPY_CHECKS) isn't actually implemented
> on x86_64 and thus we aren't catching potential security holes
> at compile time.
>
> This series adds support for strict user copy checks on x86_64
> and silences all the benign warnings in the x86_64 allyesconfig.
>
> The final patch consolidates the config option as its duplicated
> across mutliple arches. I don't know what tree this series should
> go through so I tried to send the individual driver patches to the
> respective maintainers.
>
> Stephen Boyd (9):
>   iwlegacy: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning
>   iwlwifi: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning
>   [SCSI] lpfc: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning
>   debugfs: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning
>   kprobes: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning
>   Bluetooth: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning
>   ASoC: Silence DEBUG_STRICT_USER_COPY_CHECKS=y warning
>   x86: Implement strict user copy checks for x86_64
>   Consolidate CONFIG_DEBUG_STRICT_USER_COPY_CHECKS

It looks like 1, 2, 4, 6, and 7 got picked up. Should I resend the left
over patches with appropriate acked-bys and tags? Would it be
appropriate to push this through your tree?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 0/9] strict user copy checks on x86_64

[H. Peter Anvin]

On 05/24/2011 02:29 PM, Stephen Boyd wrote:
> 
> It looks like 1, 2, 4, 6, and 7 got picked up. Should I resend the left
> over patches with appropriate acked-bys and tags? Would it be
> appropriate to push this through your tree?
> 

I was first going to think I'd pick up 8 and 9 in tip, but since 9 is
cross-architecture, Andrew's tree might be better.

Acked-by: H. Peter Anvin <hpa@zytor.com>

	-hpa