linux-kernel - Re: [RESEND PATCH] i2c: s3c2410: fix possible NULL pointer deref on read message after write

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:   Fri, 28 May 2021 10:17:42 +0200
From:   Wolfram Sang <wsa@...nel.org>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
Cc:     linux-i2c@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Sylwester Nawrocki <snawrocki@...nel.org>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        Tomasz Figa <tomasz.figa@...il.com>
Subject: Re: [RESEND PATCH] i2c: s3c2410: fix possible NULL pointer deref on
 read message after write

On Wed, May 26, 2021 at 08:39:37AM -0400, Krzysztof Kozlowski wrote:
> Interrupt handler processes multiple message write requests one after
> another, till the driver message queue is drained.  However if driver
> encounters a read message without preceding START, it stops the I2C
> transfer as it is an invalid condition for the controller.  At least the
> comment describes a requirement "the controller forces us to send a new
> START when we change direction".  This stop results in clearing the
> message queue (i2c->msg = NULL).
> 
> The code however immediately jumped back to label "retry_write" which
> dereferenced the "i2c->msg" making it a possible NULL pointer
> dereference.
> 
> The Coverity analysis:
> 1. Condition !is_msgend(i2c), taking false branch.
>    if (!is_msgend(i2c)) {
> 
> 2. Condition !is_lastmsg(i2c), taking true branch.
>    } else if (!is_lastmsg(i2c)) {
> 
> 3. Condition i2c->msg->flags & 1, taking true branch.
>    if (i2c->msg->flags & I2C_M_RD) {
> 
> 4. write_zero_model: Passing i2c to s3c24xx_i2c_stop, which sets i2c->msg to NULL.
>    s3c24xx_i2c_stop(i2c, -EINVAL);
> 
> 5. Jumping to label retry_write.
>    goto retry_write;
> 
> 6. var_deref_model: Passing i2c to is_msgend, which dereferences null i2c->msg.
>    if (!is_msgend(i2c)) {"
> 
> All previous calls to s3c24xx_i2c_stop() in this interrupt service
> routine are followed by jumping to end of function (acknowledging
> the interrupt and returning).  This seems a reasonable choice also here
> since message buffer was entirely emptied.
> 
> Addresses-Coverity: Explicit null dereferenced
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
> 

Applied to for-current, thanks!


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)