[V9fs-developer] [PATCH] Integer underflow in pdu_read()

[V9fs-developer] [PATCH] Integer underflow in pdu_read()

[Tomas Bortoli]

The pdu_read() function suffers from an integer underflow.
When pdu->offset is greater than pdu->size, the length calculation will have
a wrong result, resulting in an out-of-bound read.
This patch modifies also pdu_write() in the same way to prevent the same
issue from happening there and for consistency.

Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
Reported-by: syzbot+65c6b72f284a39d416b4@syzkaller.appspotmail.com
---
 net/9p/protocol.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/9p/protocol.c b/net/9p/protocol.c
index 931ea00c4fed..f1e2425f920b 100644
--- a/net/9p/protocol.c
+++ b/net/9p/protocol.c
@@ -55,16 +55,20 @@ EXPORT_SYMBOL(p9stat_free);
 
 size_t pdu_read(struct p9_fcall *pdu, void *data, size_t size)
 {
-	size_t len = min(pdu->size - pdu->offset, size);
-	memcpy(data, &pdu->sdata[pdu->offset], len);
+	size_t len = pdu->offset > pdu->size ? 0 :
+	 min(pdu->size - pdu->offset, size);
+	if (len != 0)
+		memcpy(data, &pdu->sdata[pdu->offset], len);
 	pdu->offset += len;
 	return size - len;
 }
 
 static size_t pdu_write(struct p9_fcall *pdu, const void *data, size_t size)
 {
-	size_t len = min(pdu->capacity - pdu->size, size);
-	memcpy(&pdu->sdata[pdu->size], data, len);
+	size_t len = pdu->size > pdu->capacity ? 0 :
+	 min(pdu->capacity - pdu->size, size);
+	if (len != 0)
+		memcpy(&pdu->sdata[pdu->size], data, len);
 	pdu->size += len;
 	return size - len;
 }
-- 
2.11.0

Re: [V9fs-developer] [PATCH] Integer underflow in pdu_read()

[Al Viro]

On Mon, Jul 09, 2018 at 09:26:51PM +0200, Tomas Bortoli wrote:
> The pdu_read() function suffers from an integer underflow.
> When pdu->offset is greater than pdu->size, the length calculation will have
> a wrong result, resulting in an out-of-bound read.
> This patch modifies also pdu_write() in the same way to prevent the same
> issue from happening there and for consistency.

What does cause the calls of pdu_read() in such conditions and shouldn't *that*
be dealt with?

Re: [V9fs-developer] [PATCH] Integer underflow in pdu_read()

[Tomas Bortoli]

On 07/09/2018 09:31 PM, Al Viro wrote:
> On Mon, Jul 09, 2018 at 09:26:51PM +0200, Tomas Bortoli wrote:
>> The pdu_read() function suffers from an integer underflow.
>> When pdu->offset is greater than pdu->size, the length calculation will have
>> a wrong result, resulting in an out-of-bound read.
>> This patch modifies also pdu_write() in the same way to prevent the same
>> issue from happening there and for consistency.
> What does cause the calls of pdu_read() in such conditions and shouldn't *that*
> be dealt with?
Mmh I think that's caused by p9_parse_header(). That function reads the
first 7 bytes of a PDU regardless of the current offset. It then sets
the PDU length to the one read and then it restores the original offset.
Therefore, it's possible to set a size < offset here.
(to be 100% sure I'd need more debugging)

We can prevent it in p9_parse_header(), but if the invariant offset <
size gets broken elsewhere we fall back to the underflow. Enforcing it
in pdu_read() should be the safest thing to do as it would detect *any*
bad read.

Re: [V9fs-developer] [PATCH] Integer underflow in pdu_read()

[Dominique Martinet]

Tomas Bortoli wrote on Tue, Jul 10, 2018:
> > What does cause the calls of pdu_read() in such conditions and shouldn't *that*
> > be dealt with?
>
> Mmh I think that's caused by p9_parse_header(). That function reads the
> first 7 bytes of a PDU regardless of the current offset. It then sets
> the PDU length to the one read and then it restores the original offset.
> Therefore, it's possible to set a size < offset here.
> (to be 100% sure I'd need more debugging)

It doesn't always restore the original offset; but if the packet lied
and said its size is < 7 it doesn't even need to.

I think as things stand it should be enough to fix it there, once the
state machine is runnning there don't seem to be any way of making
offset jump over size; but I'm not fussy either way, protecting in
pdu_read is probably just as good.
Note that there is another "min_t(uint32_t, *count, pdu->size -
pdu->offset)" that needs a similar check below in the file if you want
to go this way.

On the other hand, pdu_write() calls come from the system so hopefully
these are a bit more trustworthy, but I guess the extra check could
protect against a programming error.
I'm not sure what the linux kernel policy about these "redundant"
low-level checks is as this is technically in the fast path.

 
> We can prevent it in p9_parse_header(), but if the invariant offset <
> size gets broken elsewhere we fall back to the underflow. Enforcing it
> in pdu_read() should be the safest thing to do as it would detect *any*
> bad read.

The main problem is that 9p is just too trusty of what is sent to it.
To further extent what was said in the other thread ("[PATCH] KASAN:
slab-out-of-bounds Read in pdu_read") the extra check on pdu->size that
must be smaller than actual read size holds for p9_check_zc_error as
well so should probably be moved to p9_parse_header() ; but a packet
that says its size is < 7 is also wrong : for trans_fd, we are guaranted
to have read as much by the transport, so once again size didn't match
up.
Ideally, pdu->size should only ever be set by the transport who know
what they have read, and p9_parse_header should check that r_size ==
pdu->size and error if not.


Also, as TCP is a stream protocol, once we've had a single packet lie
about its size we're lost in the stream with no chance of recovering so
the connection should probably be aborted.
For rdma/virtio there is a notion of packet so they could recover, but
TCP is not as forgiving...

-- 
Dominique Martinet | Asmadeus

Re: [V9fs-developer] [PATCH] Integer underflow in pdu_read()

[piaojun]

Hi Tomas,

It looks like pdu->size should always be greater than pdu->offset, right?
My question may be very easy for you, please help explaining.

Thanks,
Jun

On 2018/7/10 3:26, Tomas Bortoli wrote:
> The pdu_read() function suffers from an integer underflow.
> When pdu->offset is greater than pdu->size, the length calculation will have
> a wrong result, resulting in an out-of-bound read.
> This patch modifies also pdu_write() in the same way to prevent the same
> issue from happening there and for consistency.
> 
> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
> Reported-by: syzbot+65c6b72f284a39d416b4@syzkaller.appspotmail.com
> ---
>  net/9p/protocol.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/9p/protocol.c b/net/9p/protocol.c
> index 931ea00c4fed..f1e2425f920b 100644
> --- a/net/9p/protocol.c
> +++ b/net/9p/protocol.c
> @@ -55,16 +55,20 @@ EXPORT_SYMBOL(p9stat_free);
>  
>  size_t pdu_read(struct p9_fcall *pdu, void *data, size_t size)
>  {
> -	size_t len = min(pdu->size - pdu->offset, size);
> -	memcpy(data, &pdu->sdata[pdu->offset], len);
> +	size_t len = pdu->offset > pdu->size ? 0 :
> +	 min(pdu->size - pdu->offset, size);
> +	if (len != 0)
> +		memcpy(data, &pdu->sdata[pdu->offset], len);
>  	pdu->offset += len;
>  	return size - len;
>  }
>  
>  static size_t pdu_write(struct p9_fcall *pdu, const void *data, size_t size)
>  {
> -	size_t len = min(pdu->capacity - pdu->size, size);
> -	memcpy(&pdu->sdata[pdu->size], data, len);
> +	size_t len = pdu->size > pdu->capacity ? 0 :
> +	 min(pdu->capacity - pdu->size, size);
> +	if (len != 0)
> +		memcpy(&pdu->sdata[pdu->size], data, len);
>  	pdu->size += len;
>  	return size - len;
>  }
> 

Re: [V9fs-developer] [PATCH] Integer underflow in pdu_read()

[Tomas Bortoli]

Hi Jun,

Intuitively, if you have a packet of size x and you read at an offset y,
when y>x you are off the packet. That's an out out bound read.

In this specific code when offset > size, the available length
estimation will fail as there will be an underflow resulting from
offset-size (it'll give a big big number) that breaks the out-of-bound
control put in place (if offset-size is a big big number, the asked size
to read will be probably smaller and therefore allowed).

These definitions might help:
https://cwe.mitre.org/data/definitions/787.html
https://cwe.mitre.org/data/definitions/125.html

Tomas
> Hi Tomas,
>
> It looks like pdu->size should always be greater than pdu->offset, right?
> My question may be very easy for you, please help explaining.
>
> Thanks,
> Jun
>
> On 2018/7/10 3:26, Tomas Bortoli wrote:
>> The pdu_read() function suffers from an integer underflow.
>> When pdu->offset is greater than pdu->size, the length calculation will have
>> a wrong result, resulting in an out-of-bound read.
>> This patch modifies also pdu_write() in the same way to prevent the same
>> issue from happening there and for consistency.
>>
>> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
>> Reported-by: syzbot+65c6b72f284a39d416b4@syzkaller.appspotmail.com
>> ---
>>  net/9p/protocol.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/9p/protocol.c b/net/9p/protocol.c
>> index 931ea00c4fed..f1e2425f920b 100644
>> --- a/net/9p/protocol.c
>> +++ b/net/9p/protocol.c
>> @@ -55,16 +55,20 @@ EXPORT_SYMBOL(p9stat_free);
>>  
>>  size_t pdu_read(struct p9_fcall *pdu, void *data, size_t size)
>>  {
>> -	size_t len = min(pdu->size - pdu->offset, size);
>> -	memcpy(data, &pdu->sdata[pdu->offset], len);
>> +	size_t len = pdu->offset > pdu->size ? 0 :
>> +	 min(pdu->size - pdu->offset, size);
>> +	if (len != 0)
>> +		memcpy(data, &pdu->sdata[pdu->offset], len);
>>  	pdu->offset += len;
>>  	return size - len;
>>  }
>>  
>>  static size_t pdu_write(struct p9_fcall *pdu, const void *data, size_t size)
>>  {
>> -	size_t len = min(pdu->capacity - pdu->size, size);
>> -	memcpy(&pdu->sdata[pdu->size], data, len);
>> +	size_t len = pdu->size > pdu->capacity ? 0 :
>> +	 min(pdu->capacity - pdu->size, size);
>> +	if (len != 0)
>> +		memcpy(&pdu->sdata[pdu->size], data, len);
>>  	pdu->size += len;
>>  	return size - len;
>>  }
>>

Re: [V9fs-developer] [PATCH] Integer underflow in pdu_read()

[piaojun]

Hi Tomas,

Thanks for your explaination, and I get your point.

On 2018/7/10 16:27, Tomas Bortoli wrote:
> Hi Jun,
> 
> Intuitively, if you have a packet of size x and you read at an offset y,
> when y>x you are off the packet. That's an out out bound read.
> 
> In this specific code when offset > size, the available length
> estimation will fail as there will be an underflow resulting from
> offset-size (it'll give a big big number) that breaks the out-of-bound
> control put in place (if offset-size is a big big number, the asked size
> to read will be probably smaller and therefore allowed).
> 
> These definitions might help:
> https://cwe.mitre.org/data/definitions/787.html
> https://cwe.mitre.org/data/definitions/125.html
> 
> Tomas
>> Hi Tomas,
>>
>> It looks like pdu->size should always be greater than pdu->offset, right?
>> My question may be very easy for you, please help explaining.
>>
>> Thanks,
>> Jun
>>
>> On 2018/7/10 3:26, Tomas Bortoli wrote:
>>> The pdu_read() function suffers from an integer underflow.
>>> When pdu->offset is greater than pdu->size, the length calculation will have
>>> a wrong result, resulting in an out-of-bound read.
>>> This patch modifies also pdu_write() in the same way to prevent the same
>>> issue from happening there and for consistency.
>>>
>>> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
>>> Reported-by: syzbot+65c6b72f284a39d416b4@syzkaller.appspotmail.com
>>> ---
>>>  net/9p/protocol.c | 12 ++++++++----
>>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/net/9p/protocol.c b/net/9p/protocol.c
>>> index 931ea00c4fed..f1e2425f920b 100644
>>> --- a/net/9p/protocol.c
>>> +++ b/net/9p/protocol.c
>>> @@ -55,16 +55,20 @@ EXPORT_SYMBOL(p9stat_free);
>>>  
>>>  size_t pdu_read(struct p9_fcall *pdu, void *data, size_t size)
>>>  {
>>> -	size_t len = min(pdu->size - pdu->offset, size);
>>> -	memcpy(data, &pdu->sdata[pdu->offset], len);
>>> +	size_t len = pdu->offset > pdu->size ? 0 :
>>> +	 min(pdu->size - pdu->offset, size);
>>> +	if (len != 0)
>>> +		memcpy(data, &pdu->sdata[pdu->offset], len);
>>>  	pdu->offset += len;
>>>  	return size - len;
>>>  }
>>>  
>>>  static size_t pdu_write(struct p9_fcall *pdu, const void *data, size_t size)
>>>  {
>>> -	size_t len = min(pdu->capacity - pdu->size, size);
>>> -	memcpy(&pdu->sdata[pdu->size], data, len);
>>> +	size_t len = pdu->size > pdu->capacity ? 0 :
>>> +	 min(pdu->capacity - pdu->size, size);
>>> +	if (len != 0)
>>> +		memcpy(&pdu->sdata[pdu->size], data, len);
>>>  	pdu->size += len;
>>>  	return size - len;
>>>  }
>>>
> 
> 
> 

Re: [V9fs-developer] [PATCH] Integer underflow in pdu_read()

[piaojun]

Hi Tomas,

I found some other subtraction between pdu->size and pdu->offset such as:
p9pdu_vreadf()
--min_t(uint32_t, *count, pdu->size - pdu->offset);
p9_check_zc_errors()
--len = req->rc->size - req->rc->offset;

I wonder if there are underflow risks?

Thanks,
Jun

On 2018/7/10 3:26, Tomas Bortoli wrote:
> The pdu_read() function suffers from an integer underflow.
> When pdu->offset is greater than pdu->size, the length calculation will have
> a wrong result, resulting in an out-of-bound read.
> This patch modifies also pdu_write() in the same way to prevent the same
> issue from happening there and for consistency.
> 
> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
> Reported-by: syzbot+65c6b72f284a39d416b4@syzkaller.appspotmail.com
> ---
>  net/9p/protocol.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/9p/protocol.c b/net/9p/protocol.c
> index 931ea00c4fed..f1e2425f920b 100644
> --- a/net/9p/protocol.c
> +++ b/net/9p/protocol.c
> @@ -55,16 +55,20 @@ EXPORT_SYMBOL(p9stat_free);
>  
>  size_t pdu_read(struct p9_fcall *pdu, void *data, size_t size)
>  {
> -	size_t len = min(pdu->size - pdu->offset, size);
> -	memcpy(data, &pdu->sdata[pdu->offset], len);
> +	size_t len = pdu->offset > pdu->size ? 0 :
> +	 min(pdu->size - pdu->offset, size);
> +	if (len != 0)
> +		memcpy(data, &pdu->sdata[pdu->offset], len);
>  	pdu->offset += len;
>  	return size - len;
>  }
>  
>  static size_t pdu_write(struct p9_fcall *pdu, const void *data, size_t size)
>  {
> -	size_t len = min(pdu->capacity - pdu->size, size);
> -	memcpy(&pdu->sdata[pdu->size], data, len);
> +	size_t len = pdu->size > pdu->capacity ? 0 :
> +	 min(pdu->capacity - pdu->size, size);
> +	if (len != 0)
> +		memcpy(&pdu->sdata[pdu->size], data, len);
>  	pdu->size += len;
>  	return size - len;
>  }
> 

Re: [V9fs-developer] [PATCH] Integer underflow in pdu_read()

[jiangyiwen]

On 2018/7/10 3:26, Tomas Bortoli wrote:
> The pdu_read() function suffers from an integer underflow.
> When pdu->offset is greater than pdu->size, the length calculation will have
> a wrong result, resulting in an out-of-bound read.
> This patch modifies also pdu_write() in the same way to prevent the same
> issue from happening there and for consistency.

I guess this case may happened only when server send wrong size to
the client and then cause size < offset, or else I think this case
will not happen. Is it right? Or other cases?

In addition, the email should also send to andrew morton, because
9p maintainer already don't maintain the project, andrew can help
merge the patch.

> 
> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
> Reported-by: syzbot+65c6b72f284a39d416b4@syzkaller.appspotmail.com
> ---
>  net/9p/protocol.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/9p/protocol.c b/net/9p/protocol.c
> index 931ea00c4fed..f1e2425f920b 100644
> --- a/net/9p/protocol.c
> +++ b/net/9p/protocol.c
> @@ -55,16 +55,20 @@ EXPORT_SYMBOL(p9stat_free);
>  
>  size_t pdu_read(struct p9_fcall *pdu, void *data, size_t size)
>  {
> -	size_t len = min(pdu->size - pdu->offset, size);
> -	memcpy(data, &pdu->sdata[pdu->offset], len);
> +	size_t len = pdu->offset > pdu->size ? 0 :
> +	 min(pdu->size - pdu->offset, size);

I suggest this add two *Tab* lens.

> +	if (len != 0)
> +		memcpy(data, &pdu->sdata[pdu->offset], len);
>  	pdu->offset += len;
>  	return size - len;
>  }
>  
>  static size_t pdu_write(struct p9_fcall *pdu, const void *data, size_t size)
>  {
> -	size_t len = min(pdu->capacity - pdu->size, size);
> -	memcpy(&pdu->sdata[pdu->size], data, len);
> +	size_t len = pdu->size > pdu->capacity ? 0 :
> +	 min(pdu->capacity - pdu->size, size);

The same as above.

> +	if (len != 0)
> +		memcpy(&pdu->sdata[pdu->size], data, len);
>  	pdu->size += len;
>  	return size - len;
>  }
> 

Re: [V9fs-developer] [PATCH] Integer underflow in pdu_read()

[Tomas Bortoli]

On 07/11/2018 04:04 AM, jiangyiwen wrote:
> On 2018/7/10 3:26, Tomas Bortoli wrote:
>> The pdu_read() function suffers from an integer underflow.
>> When pdu->offset is greater than pdu->size, the length calculation will have
>> a wrong result, resulting in an out-of-bound read.
>> This patch modifies also pdu_write() in the same way to prevent the same
>> issue from happening there and for consistency.
> I guess this case may happened only when server send wrong size to
> the client and then cause size < offset, or else I think this case
> will not happen. Is it right? Or other cases?
>
> In addition, the email should also send to andrew morton, because
> 9p maintainer already don't maintain the project, andrew can help
> merge the patch.
>
>> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
>> Reported-by: syzbot+65c6b72f284a39d416b4@syzkaller.appspotmail.com
>> ---
>>  net/9p/protocol.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/9p/protocol.c b/net/9p/protocol.c
>> index 931ea00c4fed..f1e2425f920b 100644
>> --- a/net/9p/protocol.c
>> +++ b/net/9p/protocol.c
>> @@ -55,16 +55,20 @@ EXPORT_SYMBOL(p9stat_free);
>>  
>>  size_t pdu_read(struct p9_fcall *pdu, void *data, size_t size)
>>  {
>> -	size_t len = min(pdu->size - pdu->offset, size);
>> -	memcpy(data, &pdu->sdata[pdu->offset], len);
>> +	size_t len = pdu->offset > pdu->size ? 0 :
>> +	 min(pdu->size - pdu->offset, size);
> I suggest this add two *Tab* lens.
>
>> +	if (len != 0)
>> +		memcpy(data, &pdu->sdata[pdu->offset], len);
>>  	pdu->offset += len;
>>  	return size - len;
>>  }
>>  
>>  static size_t pdu_write(struct p9_fcall *pdu, const void *data, size_t size)
>>  {
>> -	size_t len = min(pdu->capacity - pdu->size, size);
>> -	memcpy(&pdu->sdata[pdu->size], data, len);
>> +	size_t len = pdu->size > pdu->capacity ? 0 :
>> +	 min(pdu->capacity - pdu->size, size);
> The same as above.
>
>> +	if (len != 0)
>> +		memcpy(&pdu->sdata[pdu->size], data, len);
>>  	pdu->size += len;
>>  	return size - len;
>>  }
>>
>

This patch is not necessary anymore, the one I just sent fixes the
length validation issue.