[PATCH] firewire: nosy: don't read packets bigger than requested

[PATCH] firewire: nosy: don't read packets bigger than requested

[Jann Horn]

In general, accessing userspace memory beyond the length of the supplied
buffer in VFS read/write handlers can lead to both kernel memory corruption
(via kernel_read()/kernel_write(), which can e.g. be triggered via
sys_splice()) and privilege escalation inside userspace.

Fixes: 286468210d83 ("firewire: new driver: nosy - IEEE 1394 traffic sniffer")
Signed-off-by: Jann Horn <jannh@google.com>
---
No CC stable because this device shouldn't be available to unprivileged
code by default and should be pretty rare.

 drivers/firewire/nosy.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/firewire/nosy.c b/drivers/firewire/nosy.c
index a128dd1126ae..732075fc312e 100644
--- a/drivers/firewire/nosy.c
+++ b/drivers/firewire/nosy.c
@@ -161,11 +161,12 @@ packet_buffer_get(struct client *client, char __user *data, size_t user_length)
 	if (atomic_read(&buffer->size) == 0)
 		return -ENODEV;
 
-	/* FIXME: Check length <= user_length. */
-
 	end = buffer->data + buffer->capacity;
 	length = buffer->head->length;
 
+	if (length > user_length)
+		return -EINVAL;
+
 	if (&buffer->head->data[length] < end) {
 		if (copy_to_user(data, buffer->head->data, length))
 			return -EFAULT;
-- 
2.18.0.399.gad0ab374a1-goog

Re: [PATCH] firewire: nosy: don't read packets bigger than requested

[Jann Horn]

On Fri, Jul 6, 2018 at 5:16 PM Jann Horn <jannh@google.com> wrote:
> In general, accessing userspace memory beyond the length of the supplied
> buffer in VFS read/write handlers can lead to both kernel memory corruption
> (via kernel_read()/kernel_write(), which can e.g. be triggered via
> sys_splice()) and privilege escalation inside userspace.
>
> Fixes: 286468210d83 ("firewire: new driver: nosy - IEEE 1394 traffic sniffer")
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
> No CC stable because this device shouldn't be available to unprivileged
> code by default and should be pretty rare.
>
>  drivers/firewire/nosy.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firewire/nosy.c b/drivers/firewire/nosy.c
> index a128dd1126ae..732075fc312e 100644
> --- a/drivers/firewire/nosy.c
> +++ b/drivers/firewire/nosy.c
> @@ -161,11 +161,12 @@ packet_buffer_get(struct client *client, char __user *data, size_t user_length)
>         if (atomic_read(&buffer->size) == 0)
>                 return -ENODEV;
>
> -       /* FIXME: Check length <= user_length. */
> -
>         end = buffer->data + buffer->capacity;
>         length = buffer->head->length;
>
> +       if (length > user_length)
> +               return -EINVAL;
> +
>         if (&buffer->head->data[length] < end) {
>                 if (copy_to_user(data, buffer->head->data, length))
>                         return -EFAULT;

Ping. I sent this about two months ago, I haven't received a reply,
and from what I can tell, it hasn't landed in any tree so far...

Re: [PATCH] firewire: nosy: don't read packets bigger than requested

[Randy Dunlap]

On 09/03/2018 08:55 AM, Jann Horn wrote:
> On Fri, Jul 6, 2018 at 5:16 PM Jann Horn <jannh@google.com> wrote:
>> In general, accessing userspace memory beyond the length of the supplied
>> buffer in VFS read/write handlers can lead to both kernel memory corruption
>> (via kernel_read()/kernel_write(), which can e.g. be triggered via
>> sys_splice()) and privilege escalation inside userspace.
>>
>> Fixes: 286468210d83 ("firewire: new driver: nosy - IEEE 1394 traffic sniffer")
>> Signed-off-by: Jann Horn <jannh@google.com>
>> ---
>> No CC stable because this device shouldn't be available to unprivileged
>> code by default and should be pretty rare.
>>
>>  drivers/firewire/nosy.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/firewire/nosy.c b/drivers/firewire/nosy.c
>> index a128dd1126ae..732075fc312e 100644
>> --- a/drivers/firewire/nosy.c
>> +++ b/drivers/firewire/nosy.c
>> @@ -161,11 +161,12 @@ packet_buffer_get(struct client *client, char __user *data, size_t user_length)
>>         if (atomic_read(&buffer->size) == 0)
>>                 return -ENODEV;
>>
>> -       /* FIXME: Check length <= user_length. */
>> -
>>         end = buffer->data + buffer->capacity;
>>         length = buffer->head->length;
>>
>> +       if (length > user_length)
>> +               return -EINVAL;
>> +
>>         if (&buffer->head->data[length] < end) {
>>                 if (copy_to_user(data, buffer->head->data, length))
>>                         return -EFAULT;
> 
> Ping. I sent this about two months ago, I haven't received a reply,
> and from what I can tell, it hasn't landed in any tree so far...
> 

:(
I have that same problem with some Firewire documentation patches.
I plan to ask someone else to merge my patches.

-- 
~Randy

Re: [PATCH] firewire: nosy: don't read packets bigger than requested

[Stefan Richter]

On Sep 03 Randy Dunlap wrote:
> On 09/03/2018 08:55 AM, Jann Horn wrote:
> > On Fri, Jul 6, 2018 at 5:16 PM Jann Horn <jannh@google.com> wrote:  
> >> In general, accessing userspace memory beyond the length of the supplied
> >> buffer in VFS read/write handlers can lead to both kernel memory corruption
> >> (via kernel_read()/kernel_write(), which can e.g. be triggered via
> >> sys_splice()) and privilege escalation inside userspace.
> >>
> >> Fixes: 286468210d83 ("firewire: new driver: nosy - IEEE 1394 traffic sniffer")
> >> Signed-off-by: Jann Horn <jannh@google.com>
[...]
> >>  drivers/firewire/nosy.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
[...]
> > Ping. I sent this about two months ago, I haven't received a reply,
> > and from what I can tell, it hasn't landed in any tree so far...
> >   
> 
> :(
> I have that same problem with some Firewire documentation patches.
> I plan to ask someone else to merge my patches.

Jann,

sorry for not responding in July (was buried in other work and been
effectively absent from maintainership for many months).  And sorry for
missing your ping in September (it must have been misplaced into the spam
folder and I apparently overlooked it there).

This week is another one in which I will not be able to check your patch.
Next week I will have a vacation of sorts and will use it to (a) review
and merge your patch and (b) clean out my mailbox and update my mail
sorting filters (long overdue after my mail service provider changed
backends).

Again sorry, and thank you for your extraordinary patience.
-- 
Stefan Richter
-======---=- =--= =--=-
http://arcgraph.de/sr/