ibm-s390-linux / smc-tools

Tools for use with AF_SMC sockets
Eclipse Public License 1.0
19 stars 14 forks source link

stats.c: possible memory read overrun in is_data_consistent #2

Closed ckalina closed 2 years ago

ckalina commented 3 years ago

Hi,

I'd like to report a possible memory read overrun detected by static analysis in function is_data_consistent, function added in https://github.com/ibm-s390-linux/smc-tools/commit/094115776fbec0f0675e229c80f5ddf156afa53a and changed to the current form by commit https://github.com/ibm-s390-linux/smc-tools/commit/472713cba41126adcc8b54855e3b76c699c8752a: https://github.com/ibm-s390-linux/smc-tools/blob/393dd23a767ab111608a534b4b0d200e45fe04f0/stats.c#L887-L922 namely the line: https://github.com/ibm-s390-linux/smc-tools/blob/393dd23a767ab111608a534b4b0d200e45fe04f0/stats.c#L906 as overrunning array of 496 bytes at byte offset 1496 by dereferencing pointer kern_fbck.

If this is expected, please close this issue. Thank you!

Minimal example

This seems to be verified using the following minimal example. The issue appears to be in the cycle; in the example read is substituted for write to illustrate memory accesses.

#include <stdint.h>
#include <string.h>
#include <stdio.h>
#include <assert.h>

typedef uint64_t __u64;

#include "smc-tools/stats.h"

const struct smc_stats smc_stat;

int __attribute__((section (".canarySection"))) canary1 = 10;
struct smc_stats_rsn __attribute__((section (".canarySection"))) smc_rsn;
int __attribute__((section (".canarySection"))) canary2 = 20;

struct smc_stats_fback *kern_fbck = (struct smc_stats_fback *)&smc_rsn;

int main(void)
{
        int i;
        int size = sizeof(smc_stat) / sizeof(uint64_t);
        int size_fback = size + 2 * SMC_MAX_FBACK_RSN_CNT;

        memset(kern_fbck, -1, sizeof(smc_rsn));

        printf("Canary1 addr %p\tval 0x%08x\n", (void*)&canary1, canary1);
        printf("Canary2 addr %p\tval 0x%08x\n", (void*)&canary2, canary2);

        printf("\n");
        printf("smc_rsn accessess:\n");
        for (i = 0; i < size_fback; i++) {
                printf("\taddr %p\tval 0x%08x\t%3s ",
                       (void*)&(kern_fbck->fback_code),
                       kern_fbck->fback_code,
                       (kern_fbck->fback_code == -1) ? "OK" : "OOB");
                kern_fbck->fback_code = 2;
                printf("~> written val 0x%08x",
                       kern_fbck->fback_code);

                if (&canary2 >= &kern_fbck->fback_code && &canary2 <= &(kern_fbck->fback_code)+1)
                        printf("\tCANARY");
                printf("\n");

                kern_fbck++;
        }

        printf("\n");
        printf("Canary1 addr %p\tval 0x%08x\n", (void*)&canary1, canary1);
        printf("Canary2 addr %p\tval 0x%08x\n", (void*)&canary2, canary2);
        printf("\n");

        assert(canary1 == 10);
        assert(canary2 == 20);

        return 0;
}
SECTIONS
{
        .canarySegment 0x2000000 : {KEEP(*(.canarySection))}
}

Compiled with gcc -fsanitize=address -fstack-protector-all -fno-pie -no-pie -Wall -Wextra -Werror -pedantic -std=gnu99 ./link.ld ./a.c -o ./a

Sample output (snipped):

Canary1 addr 0x2000000  val 0x0000000a
Canary2 addr 0x20001f8  val 0x00000014

smc_rsn accessess:
        addr 0x2000008  val 0xffffffff   OK ~> written val 0x00000002
        ... legitimate memory accessess ...
        addr 0x20001f0  val 0xffffffff   OK ~> written val 0x00000002
        addr 0x20001f8  val 0x00000014  OOB ~> written val 0x00000002   CANARY
        ... overrun follows ...
        addr 0x2000420  val 0x0000000b  OOB ~> written val 0x00000002
        addr 0x2000428  val 0x00000100  OOB ~>

a: ./a.c:54: main: Assertion `canary2 == 20' failed.
Stefan-Raspl commented 3 years ago

@ckalina Thanks for reporting! Should be fixed in commit 011fc85 "_smc-tools: stats: Fix memory overread in is_dataconsistent()".