linux-kernel - Re: Possible race in xilinx-xadc.ko

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 19 Aug 2017 09:26:22 +0200
From:   Lars-Peter Clausen <lars@...afoo.de>
To:     Anton Volkov <avolkov@...ras.ru>
Cc:     jic23@...nel.org, knaack.h@....de, pmeerw@...erw.net,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        ldv-project@...uxtesting.org,
        Alexey Khoroshilov <khoroshilov@...ras.ru>
Subject: Re: Possible race in xilinx-xadc.ko

On 08/18/2017 05:10 PM, Anton Volkov wrote:
> Hello.
> 
> While searching for races in the Linux kernel I've come across
> "drivers/iio/adc/xilinx-xadc.ko" module. Here is a question that I came up
> with while analyzing results. Lines are given using the info from Linux v4.12.
> 
> Consider the following case:
> 
> Thread 1:                    Thread 2:
> xadc_probe
>   iio_triggered_buffer_setup
>                              xadc_trigger_handler
>                              ->xadc_read_adc_reg
>                                ->_xadc_read_adc_reg
>                                  ->xadc_zynq_read_adc_reg
> ->xadc_zynq_setup                  ->xadc_zynq_update_intmsk
>    xadc->zynq_intmask = ~0            xadc->zynq_intmask &= ~mask
>    (xilinx-xadc-core.c: line 340)     (xilinx-xadc-core.c: line 152)
>    xadc_write_reg(xadc->zynq_intmask) xadc_write_reg(xadc->zynq_intmask)
> 
> Judging by the comments to iio_triggered_buffer_setup() the best practice is
> to call it after the indio_dev structure had been fully initialized. In the
> suggested case xadc_trigger_handler() interferes in the initialization
> process. Both xadc_zynq_setup() and xadc_zynq_update_intmsk() will write the
> same xadc->zynq_intmask though it should probably be different. Is this race
> feasible from your point of view?

Hi,

Thanks for the report. I don't think there is an issue here. All
iio_triggered_buffer_setup() does is store the pointer to the callback
function in the iio_poll_func structure of the device. The callback is not
live until iio_trigger_attach_poll_func() has been invoked, which can only
happen after iio_device_register() has completed.

- Lars