[PATCH] tty: fix data race in tty_buffer_flush

[PATCH] tty: fix data race in tty_buffer_flush

[Dmitry Vyukov]

tty_buffer_flush frees not acquired buffers.
As the result, for example, read of b->size in tty_buffer_free
can return garbage value which will lead to a huge buffer
hanging in the freelist. This is just the benignest
manifestation of freeing of a not acquired object.
If the object is passed to kfree, heap can be corrupted.

Acquire visibility over the buffer before freeing it.

The data race was found with KernelThreadSanitizer (KTSAN).

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
---
 drivers/tty/tty_buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 5a3fa89..7dab1b3 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -242,7 +242,7 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
 	atomic_inc(&buf->priority);
 
 	mutex_lock(&buf->lock);
-	while ((next = buf->head->next) != NULL) {
+	while ((next = smp_load_acquire(&buf->head->next)) != NULL) {
 		tty_buffer_free(port, buf->head);
 		buf->head = next;
 	}
-- 
2.5.0.457.gab17608

[PATCH] tty: fix data race in tty_buffer_flush

[Dmitry Vyukov]

tty_buffer_flush frees not acquired buffers.
As the result, for example, read of b->size in tty_buffer_free
can return garbage value which will lead to a huge buffer
hanging in the freelist. This is just the benignest
manifestation of freeing of a not acquired object.
If the object is passed to kfree, heap can be corrupted.

Acquire visibility over the buffer before freeing it.

The data race was found with KernelThreadSanitizer (KTSAN).

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
---
 drivers/tty/tty_buffer.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 5a3fa89..a2a8cd0 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -242,7 +242,10 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
 	atomic_inc(&buf->priority);
 
 	mutex_lock(&buf->lock);
-	while ((next = buf->head->next) != NULL) {
+	/* paired w/ release in __tty_buffer_request_room; ensures there are
+	 * no pending memory accesses to the freed buffer
+	 */
+	while ((next = smp_load_acquire(&buf->head->next)) != NULL) {
 		tty_buffer_free(port, buf->head);
 		buf->head = next;
 	}
-- 
2.5.0.457.gab17608

Re: [PATCH] tty: fix data race in tty_buffer_flush

[Greg KH]

On Tue, Sep 08, 2015 at 02:48:26PM +0200, Dmitry Vyukov wrote:
> tty_buffer_flush frees not acquired buffers.
> As the result, for example, read of b->size in tty_buffer_free
> can return garbage value which will lead to a huge buffer
> hanging in the freelist. This is just the benignest
> manifestation of freeing of a not acquired object.
> If the object is passed to kfree, heap can be corrupted.
> 
> Acquire visibility over the buffer before freeing it.
> 
> The data race was found with KernelThreadSanitizer (KTSAN).
> 
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> ---
>  drivers/tty/tty_buffer.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

What changed from the last version?  Why should I take this one and not
the other?  What order do these go in?

Please please please version your patches, and number them in a series
so that I know what to apply, and in what order, and what changed
between versions.  Please read Documentation/SubmittingPatches for all
the details about this.

I've now dropped all of your patches from my queue, please resend them
all properly.

thanks,

greg k-h

Re: [PATCH] tty: fix data race in tty_buffer_flush

[Peter Hurley]

On Tue, Sep 8, 2015 at 8:48 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> tty_buffer_flush frees not acquired buffers.
> As the result, for example, read of b->size in tty_buffer_free
> can return garbage value which will lead to a huge buffer
> hanging in the freelist. This is just the benignest
> manifestation of freeing of a not acquired object.
> If the object is passed to kfree, heap can be corrupted.
>
> Acquire visibility over the buffer before freeing it.
>
> The data race was found with KernelThreadSanitizer (KTSAN).

Dmitry,

Looks good. Thanks for splitting this from the other fix.

Like Greg said, the patch needs revision info to help maintainers, et al
track which is most current/relevant/etc.

Typical format for $subject is something like:

[PATCH v4] tty: fix data race in tty_buffer_flush


Notes re: changes from other patch versions are added below the
git changelog separator (example below).

> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> ---

v4: Corrected commit log and patch revision notes

v3: Added code comment re: paired smp barrier

v2: Split from 'tty: fix data races on tty_buffer.commit'

>  drivers/tty/tty_buffer.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 5a3fa89..a2a8cd0 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -242,7 +242,10 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
>         atomic_inc(&buf->priority);
>
>         mutex_lock(&buf->lock);
> -       while ((next = buf->head->next) != NULL) {
> +       /* paired w/ release in __tty_buffer_request_room; ensures there are
> +        * no pending memory accesses to the freed buffer
> +        */
> +       while ((next = smp_load_acquire(&buf->head->next)) != NULL) {
>                 tty_buffer_free(port, buf->head);
>                 buf->head = next;
>         }
> --
> 2.5.0.457.gab17608
>

Re: [PATCH] tty: fix data race in tty_buffer_flush

[Dmitry Vyukov]

Sorry for the delay. It is starred in my inbox and I plan to resend
the patches. Just did not have time yet.


On Wed, Sep 16, 2015 at 3:38 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On Tue, Sep 8, 2015 at 8:48 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> tty_buffer_flush frees not acquired buffers.
>> As the result, for example, read of b->size in tty_buffer_free
>> can return garbage value which will lead to a huge buffer
>> hanging in the freelist. This is just the benignest
>> manifestation of freeing of a not acquired object.
>> If the object is passed to kfree, heap can be corrupted.
>>
>> Acquire visibility over the buffer before freeing it.
>>
>> The data race was found with KernelThreadSanitizer (KTSAN).
>
> Dmitry,
>
> Looks good. Thanks for splitting this from the other fix.
>
> Like Greg said, the patch needs revision info to help maintainers, et al
> track which is most current/relevant/etc.
>
> Typical format for $subject is something like:
>
> [PATCH v4] tty: fix data race in tty_buffer_flush
>
>
> Notes re: changes from other patch versions are added below the
> git changelog separator (example below).
>
>> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>> ---
>
> v4: Corrected commit log and patch revision notes
>
> v3: Added code comment re: paired smp barrier
>
> v2: Split from 'tty: fix data races on tty_buffer.commit'
>
>>  drivers/tty/tty_buffer.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
>> index 5a3fa89..a2a8cd0 100644
>> --- a/drivers/tty/tty_buffer.c
>> +++ b/drivers/tty/tty_buffer.c
>> @@ -242,7 +242,10 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
>>         atomic_inc(&buf->priority);
>>
>>         mutex_lock(&buf->lock);
>> -       while ((next = buf->head->next) != NULL) {
>> +       /* paired w/ release in __tty_buffer_request_room; ensures there are
>> +        * no pending memory accesses to the freed buffer
>> +        */
>> +       while ((next = smp_load_acquire(&buf->head->next)) != NULL) {
>>                 tty_buffer_free(port, buf->head);
>>                 buf->head = next;
>>         }
>> --
>> 2.5.0.457.gab17608
>>



-- 
Dmitry Vyukov, Software Engineer, dvyukov@google.com
Google Germany GmbH, Dienerstraße 12, 80331, München
Geschäftsführer: Graham Law, Christine Elizabeth Flores
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat
sind, leiten Sie diese bitte nicht weiter, informieren Sie den
Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
This e-mail is confidential. If you are not the right addressee please
do not forward it, please inform the sender, and please erase this
e-mail including any attachments. Thanks.

Re: [PATCH] tty: fix data race in tty_buffer_flush

[Dmitry Vyukov]

One stupid question. There is a number of branches:
  remotes/origin/master
  remotes/origin/tty-linus
  remotes/origin/tty-next
  remotes/origin/tty-testing

What branch should I base my patches on? I used master.




On Wed, Sep 16, 2015 at 8:20 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> Sorry for the delay. It is starred in my inbox and I plan to resend
> the patches. Just did not have time yet.
>
>
> On Wed, Sep 16, 2015 at 3:38 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On Tue, Sep 8, 2015 at 8:48 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>> tty_buffer_flush frees not acquired buffers.
>>> As the result, for example, read of b->size in tty_buffer_free
>>> can return garbage value which will lead to a huge buffer
>>> hanging in the freelist. This is just the benignest
>>> manifestation of freeing of a not acquired object.
>>> If the object is passed to kfree, heap can be corrupted.
>>>
>>> Acquire visibility over the buffer before freeing it.
>>>
>>> The data race was found with KernelThreadSanitizer (KTSAN).
>>
>> Dmitry,
>>
>> Looks good. Thanks for splitting this from the other fix.
>>
>> Like Greg said, the patch needs revision info to help maintainers, et al
>> track which is most current/relevant/etc.
>>
>> Typical format for $subject is something like:
>>
>> [PATCH v4] tty: fix data race in tty_buffer_flush
>>
>>
>> Notes re: changes from other patch versions are added below the
>> git changelog separator (example below).
>>
>>> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>>> ---
>>
>> v4: Corrected commit log and patch revision notes
>>
>> v3: Added code comment re: paired smp barrier
>>
>> v2: Split from 'tty: fix data races on tty_buffer.commit'
>>
>>>  drivers/tty/tty_buffer.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
>>> index 5a3fa89..a2a8cd0 100644
>>> --- a/drivers/tty/tty_buffer.c
>>> +++ b/drivers/tty/tty_buffer.c
>>> @@ -242,7 +242,10 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
>>>         atomic_inc(&buf->priority);
>>>
>>>         mutex_lock(&buf->lock);
>>> -       while ((next = buf->head->next) != NULL) {
>>> +       /* paired w/ release in __tty_buffer_request_room; ensures there are
>>> +        * no pending memory accesses to the freed buffer
>>> +        */
>>> +       while ((next = smp_load_acquire(&buf->head->next)) != NULL) {
>>>                 tty_buffer_free(port, buf->head);
>>>                 buf->head = next;
>>>         }
>>> --
>>> 2.5.0.457.gab17608
>>>
>
>
>
> --
> Dmitry Vyukov, Software Engineer, dvyukov@google.com
> Google Germany GmbH, Dienerstraße 12, 80331, München
> Geschäftsführer: Graham Law, Christine Elizabeth Flores
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat
> sind, leiten Sie diese bitte nicht weiter, informieren Sie den
> Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
> This e-mail is confidential. If you are not the right addressee please
> do not forward it, please inform the sender, and please erase this
> e-mail including any attachments. Thanks.



-- 
Dmitry Vyukov, Software Engineer, dvyukov@google.com
Google Germany GmbH, Dienerstraße 12, 80331, München
Geschäftsführer: Graham Law, Christine Elizabeth Flores
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat
sind, leiten Sie diese bitte nicht weiter, informieren Sie den
Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
This e-mail is confidential. If you are not the right addressee please
do not forward it, please inform the sender, and please erase this
e-mail including any attachments. Thanks.

Re: [PATCH] tty: fix data race in tty_buffer_flush

[Dmitry Vyukov]

I've resent the 3 patches with version information.

On Thu, Sep 17, 2015 at 12:28 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> One stupid question. There is a number of branches:
>   remotes/origin/master
>   remotes/origin/tty-linus
>   remotes/origin/tty-next
>   remotes/origin/tty-testing
>
> What branch should I base my patches on? I used master.
>
>
>
>
> On Wed, Sep 16, 2015 at 8:20 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> Sorry for the delay. It is starred in my inbox and I plan to resend
>> the patches. Just did not have time yet.
>>
>>
>> On Wed, Sep 16, 2015 at 3:38 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>> On Tue, Sep 8, 2015 at 8:48 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>> tty_buffer_flush frees not acquired buffers.
>>>> As the result, for example, read of b->size in tty_buffer_free
>>>> can return garbage value which will lead to a huge buffer
>>>> hanging in the freelist. This is just the benignest
>>>> manifestation of freeing of a not acquired object.
>>>> If the object is passed to kfree, heap can be corrupted.
>>>>
>>>> Acquire visibility over the buffer before freeing it.
>>>>
>>>> The data race was found with KernelThreadSanitizer (KTSAN).
>>>
>>> Dmitry,
>>>
>>> Looks good. Thanks for splitting this from the other fix.
>>>
>>> Like Greg said, the patch needs revision info to help maintainers, et al
>>> track which is most current/relevant/etc.
>>>
>>> Typical format for $subject is something like:
>>>
>>> [PATCH v4] tty: fix data race in tty_buffer_flush
>>>
>>>
>>> Notes re: changes from other patch versions are added below the
>>> git changelog separator (example below).
>>>
>>>> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>>>> ---
>>>
>>> v4: Corrected commit log and patch revision notes
>>>
>>> v3: Added code comment re: paired smp barrier
>>>
>>> v2: Split from 'tty: fix data races on tty_buffer.commit'
>>>
>>>>  drivers/tty/tty_buffer.c | 5 ++++-
>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
>>>> index 5a3fa89..a2a8cd0 100644
>>>> --- a/drivers/tty/tty_buffer.c
>>>> +++ b/drivers/tty/tty_buffer.c
>>>> @@ -242,7 +242,10 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
>>>>         atomic_inc(&buf->priority);
>>>>
>>>>         mutex_lock(&buf->lock);
>>>> -       while ((next = buf->head->next) != NULL) {
>>>> +       /* paired w/ release in __tty_buffer_request_room; ensures there are
>>>> +        * no pending memory accesses to the freed buffer
>>>> +        */
>>>> +       while ((next = smp_load_acquire(&buf->head->next)) != NULL) {
>>>>                 tty_buffer_free(port, buf->head);
>>>>                 buf->head = next;
>>>>         }
>>>> --
>>>> 2.5.0.457.gab17608
>>>>
>>
>>
>>
>> --
>> Dmitry Vyukov, Software Engineer, dvyukov@google.com
>> Google Germany GmbH, Dienerstraße 12, 80331, München
>> Geschäftsführer: Graham Law, Christine Elizabeth Flores
>> Registergericht und -nummer: Hamburg, HRB 86891
>> Sitz der Gesellschaft: Hamburg
>> Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat
>> sind, leiten Sie diese bitte nicht weiter, informieren Sie den
>> Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
>> This e-mail is confidential. If you are not the right addressee please
>> do not forward it, please inform the sender, and please erase this
>> e-mail including any attachments. Thanks.
>
>
>
> --
> Dmitry Vyukov, Software Engineer, dvyukov@google.com
> Google Germany GmbH, Dienerstraße 12, 80331, München
> Geschäftsführer: Graham Law, Christine Elizabeth Flores
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat
> sind, leiten Sie diese bitte nicht weiter, informieren Sie den
> Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
> This e-mail is confidential. If you are not the right addressee please
> do not forward it, please inform the sender, and please erase this
> e-mail including any attachments. Thanks.



-- 
Dmitry Vyukov, Software Engineer, dvyukov@google.com
Google Germany GmbH, Dienerstraße 12, 80331, München
Geschäftsführer: Graham Law, Christine Elizabeth Flores
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat
sind, leiten Sie diese bitte nicht weiter, informieren Sie den
Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
This e-mail is confidential. If you are not the right addressee please
do not forward it, please inform the sender, and please erase this
e-mail including any attachments. Thanks.

Re: [PATCH] tty: fix data race in tty_buffer_flush

[Greg Kroah-Hartman]

On Thu, Sep 17, 2015 at 12:28:11PM +0200, Dmitry Vyukov wrote:
> One stupid question. There is a number of branches:
>   remotes/origin/master
>   remotes/origin/tty-linus
>   remotes/origin/tty-next
>   remotes/origin/tty-testing
> 
> What branch should I base my patches on? I used master.

Right now, if you look, they all point to the same thing, so it doesn't
matter :)

tty-linus is for patches to go to Linus this kernel release.
tty-next is for patches to go to Linus the next kernel release.

Both tty-linus and tty-next show up in linux-next and are never rebased.

tty-testing is for patches that are being considered for tty-next, it
does not show up in linux-next and sometimes is rebased if things go
wrong with the patches.

Hope this helps,

greg k-h