linux-kernel - Re: [PATCH v2] EDAC: expose per-dimm error counts in sysfs

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <7EB49132-0CB0-46AE-8F2C-1AF334DE4494@fb.com>
Date:   Fri, 28 Oct 2016 18:13:08 +0000
From:   Aaron Miller <aaronmiller@...com>
To:     Borislav Petkov <bp@...en8.de>,
        Mauro Carvalho Chehab <mchehab@...nel.org>
CC:     "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] EDAC: expose per-dimm error counts in sysfs

The EDAC drivers themselves are reporting the channel and slot counts and dimm config and also doing the decoding of MCEs to map them to a node/channel/slot. If one of those was reporting that an MCE came from a position it had reported earlier did not exist I would say that driver was buggy, since the MCE doesn’t contain those as raw values from h/w, they have to be determined by the driver using what it knows about the current memory configuration.

Adding some bound checks might not be a bad idea in this does happen, but you can cause an oops like this by injecting out of bounds without this patch – only added reads it there shouldn’t be a way to cause an out-of-bounds read with it.

On 10/28/16, 6:02 AM, "Borislav Petkov" <bp@...en8.de> wrote:

    On Thu, Oct 27, 2016 at 02:33:32PM -0700, Aaron Miller wrote:
    > The old 'csrowX' sysfs directories had per-csrow error counters, but the
    > new 'dimmX' directories do not currently expose error counts.
    > 
    > EDAC already keeps these counts, add them to sysfs so per-dimm counts
    > are still available when CONFIG_EDAC_LEGACY_SYSFS=n
    > 
    > Signed-off-by: Aaron Miller <aaronmiller@...com>
    > ---
    
    Hmm, so something's still broken in the adding of the error count,
    especially that dancing around with layers.
    
    Mauro, I think you should take a look. Here's what I did:
    
    EDAC_DFS=/sys/kernel/debug/edac/mc0/
    
    echo 24 > $EDAC_DFS/fake_inject_channel
    echo 0 > $EDAC_DFS/fake_inject_slot
    echo 3 > $EDAC_DFS/fake_inject_count
    
    echo 1 > $EDAC_DFS/fake_inject
    
    24 is the max channels the system reports during start:
    
    [ 4864.030901] EDAC MC: Removed device 0 for sbridge_edac.c Sandy Bridge Socket#0: DEV 0000:3f:0e.0
    [ 4865.867873] EDAC MC: Ver: 3.0.0
    [ 4866.081102] edac_mc_alloc: errcount layer 0 size 8
    [ 4866.086081] edac_mc_alloc: errcount layer 1 size 24
    [ 4866.091133] edac_mc_alloc: allocating 64 error counters
    [ 4866.096529] edac_mc_alloc: allocating 3320 bytes for mci data (24 dimms, 24 csrows/channels)
    								  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    
    [ 4866.105146] edac_mc_alloc: ce_per_layer[0]: ffff8928b7e918f8
    [ 4866.110979] edac_mc_alloc: ce_per_layer[1]: ffff8928b7e91938
    [ 4866.619521] slot 0 <7>[ 4866.621530] EDAC DEBUG: edac_create_dimm_object: creating rank/dimm device dimm0
    [ 4866.637405] slot 0 <7>[ 4866.639420] EDAC DEBUG: edac_create_dimm_object: creating rank/dimm device dimm3
    [ 4866.655282] slot 0 <7>[ 4866.657288] EDAC DEBUG: edac_create_dimm_object: creating rank/dimm device dimm6
    [ 4866.673168] slot 0 <7>[ 4866.675190] EDAC DEBUG: edac_create_dimm_object: creating rank/dimm device dimm9
    [ 4866.691114] EDAC MC0: Giving out device to module sbridge_edac.c controller Sandy Bridge Socket#0: DEV 0000:3f:0e.0 (INTERRUPT)
    
    and when I do that injection, there's boom, see below.
    
    It looks to me that indexing in edac_inc_ce_error() goes out of bounds:
    
                    mci->ce_per_layer[i][index] += count;
    
                    if (i < mci->n_layers - 1)
                            index *= mci->layers[i + 1].size;
    
    and it shouldn't.
    
    We need to be prepared to handle crap data coming from the hardware so
    those error count adding functions should check array lengths and scream
    if something's overflowing...