Re: [PATCH 13/18] ipv6: prevent bounds-check bypass via speculative execution

[Dan Williams]

On Sat, Jan 6, 2018 at 6:48 AM, Stephen Hemminger
 wrote:
> On Fri, 05 Jan 2018 17:11:04 -0800
> Dan Williams  wrote:
>
>> Static analysis reports that 'offset' may be a user controlled value
>> that is used as a data dependency reading from a raw6_frag_vec buffer.
>> In order to avoid potential leaks of kernel memory values, block
>> speculative execution of the instruction stream that could issue further
>> reads based on an invalid '*(rfv->c + offset)' value.
>>
>> Based on an original patch by Elena Reshetova.
>>
>> Cc: "David S. Miller" 
>> Cc: Alexey Kuznetsov 
>> Cc: Hideaki YOSHIFUJI 
>> Cc: netdev@vger.kernel.org
>> Signed-off-by: Elena Reshetova 
>> Signed-off-by: Dan Williams 
>> ---
>>  net/ipv6/raw.c |9 +
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
>> index 761a473a07c5..384e3d59d148 100644
>> --- a/net/ipv6/raw.c
>> +++ b/net/ipv6/raw.c
>> @@ -33,6 +33,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>
>>  #include 
>> @@ -725,17 +726,17 @@ static int raw6_getfrag(void *from, char *to, int 
>> offset, int len, int odd,
>>  struct sk_buff *skb)
>>  {
>>   struct raw6_frag_vec *rfv = from;
>> + char *rfv_buf;
>>
>> - if (offset < rfv->hlen) {
>> + if ((rfv_buf = nospec_array_ptr(rfv->c, offset, rfv->hlen))) {
>>   int copy = min(rfv->hlen - offset, len);
>
> Minor nit.
>
> Please don't do assignment in condition test here.
> Instead.
> rfv_buf = nospec_array_ptr(rfv->c, offset, rfv->hlen);
> if (rfv_buf) {

Yeah, sorry about that. This was a hold over from an earlier version
where nospec_array_ptr() did not include the necessary barrier and we
relied on a new if_nospec helper, but now that if_nospec is no longer
being proposed I can go back and clean this up.

Re: [PATCH 13/18] ipv6: prevent bounds-check bypass via speculative execution

[Stephen Hemminger]

On Fri, 05 Jan 2018 17:11:04 -0800
Dan Williams  wrote:

> Static analysis reports that 'offset' may be a user controlled value
> that is used as a data dependency reading from a raw6_frag_vec buffer.
> In order to avoid potential leaks of kernel memory values, block
> speculative execution of the instruction stream that could issue further
> reads based on an invalid '*(rfv->c + offset)' value.
> 
> Based on an original patch by Elena Reshetova.
> 
> Cc: "David S. Miller" 
> Cc: Alexey Kuznetsov 
> Cc: Hideaki YOSHIFUJI 
> Cc: netdev@vger.kernel.org
> Signed-off-by: Elena Reshetova 
> Signed-off-by: Dan Williams 
> ---
>  net/ipv6/raw.c |9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
> index 761a473a07c5..384e3d59d148 100644
> --- a/net/ipv6/raw.c
> +++ b/net/ipv6/raw.c
> @@ -33,6 +33,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include 
> @@ -725,17 +726,17 @@ static int raw6_getfrag(void *from, char *to, int 
> offset, int len, int odd,
>  struct sk_buff *skb)
>  {
>   struct raw6_frag_vec *rfv = from;
> + char *rfv_buf;
>  
> - if (offset < rfv->hlen) {
> + if ((rfv_buf = nospec_array_ptr(rfv->c, offset, rfv->hlen))) {
>   int copy = min(rfv->hlen - offset, len);

Minor nit.

Please don't do assignment in condition test here.
Instead.
rfv_buf = nospec_array_ptr(rfv->c, offset, rfv->hlen);
if (rfv_buf) {

Re: [PATCH 13/18] ipv6: prevent bounds-check bypass via speculative execution

[Sergei Shtylyov]

On 1/6/2018 4:11 AM, Dan Williams wrote:


Static analysis reports that 'offset' may be a user controlled value
that is used as a data dependency reading from a raw6_frag_vec buffer.
In order to avoid potential leaks of kernel memory values, block
speculative execution of the instruction stream that could issue further
reads based on an invalid '*(rfv->c + offset)' value.

Based on an original patch by Elena Reshetova.

Cc: "David S. Miller" 
Cc: Alexey Kuznetsov 
Cc: Hideaki YOSHIFUJI 
Cc: netdev@vger.kernel.org
Signed-off-by: Elena Reshetova 
Signed-off-by: Dan Williams 
---
  net/ipv6/raw.c |9 +
  1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index 761a473a07c5..384e3d59d148 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c

[...]

@@ -725,17 +726,17 @@ static int raw6_getfrag(void *from, char *to, int offset, 
int len, int odd,
   struct sk_buff *skb)
  {
struct raw6_frag_vec *rfv = from;
+   char *rfv_buf;
  
-	if (offset < rfv->hlen) {

+   if ((rfv_buf = nospec_array_ptr(rfv->c, offset, rfv->hlen))) {


   And here...


int copy = min(rfv->hlen - offset, len);
  
  		if (skb->ip_summed == CHECKSUM_PARTIAL)

-   memcpy(to, rfv->c + offset, copy);
+   memcpy(to, rfv_buf, copy);
else
skb->csum = csum_block_add(
skb->csum,
-   csum_partial_copy_nocheck(rfv->c + offset,
- to, copy, 0),
+   csum_partial_copy_nocheck(rfv_buf, to, copy, 0),
odd);
  
  		odd = 0;


MBR, Sergei