au-ts / sddf

A collection of interfaces, libraries and tools for writing device drivers for seL4 that allow accessing devices securely and with low overhead.
Other
17 stars 14 forks source link

Fix for array out of bound in init() in virt_tx.c when there is only one client #213

Open Cheng-Li1 opened 3 weeks ago

Cheng-Li1 commented 3 weeks ago

This PR introduces a small refactor to the initialization of the state.buffer_region_paddrs array. The previous init code assumes there are always two clients. This is needed because my example's eth_virt_tx only has one client.

Changes: Replaced the hardcoded initialization with a dynamic approach that uses an array of pointers to initialize state.buffer_region_paddrs. The initialization loop now iterates based on NUM_NETWORK_CLIENTS, ensuring that the code adapts to varying numbers of clients.

Ivan-Velickovic commented 3 weeks ago

net_mem_region_init_sys is also wrong.

static inline void net_mem_region_init_sys(char *pd_name, uintptr_t *mem_regions, uintptr_t start_region)
{
    if (!sddf_strcmp(pd_name, NET_VIRT_TX_NAME)) {
        mem_regions[0] = start_region;
        mem_regions[1] = start_region + NET_DATA_REGION_SIZE;
    }
}

it does this right now.

We have an invariant that the virtual addresses for the regions are contiguous, but that might not apply to the physical addresses. Not sure how to deal with that part yet.

Cheng-Li1 commented 3 weeks ago

net_mem_region_init_sys is also wrong.

static inline void net_mem_region_init_sys(char *pd_name, uintptr_t *mem_regions, uintptr_t start_region)
{
    if (!sddf_strcmp(pd_name, NET_VIRT_TX_NAME)) {
        mem_regions[0] = start_region;
        mem_regions[1] = start_region + NET_DATA_REGION_SIZE;
    }
}

it does this right now.

We have an invariant that the virtual addresses for the regions are contiguous, but that might not apply to the physical addresses. Not sure how to deal with that part yet.

are those two vaddrs? I think they manage it in the .system file by laying the two memory regions contiguous.

Cheng-Li1 commented 3 weeks ago

net_mem_region_init_sys is also wrong.

static inline void net_mem_region_init_sys(char *pd_name, uintptr_t *mem_regions, uintptr_t start_region)
{
    if (!sddf_strcmp(pd_name, NET_VIRT_TX_NAME)) {
        mem_regions[0] = start_region;
        mem_regions[1] = start_region + NET_DATA_REGION_SIZE;
    }
}

it does this right now. We have an invariant that the virtual addresses for the regions are contiguous, but that might not apply to the physical addresses. Not sure how to deal with that part yet.

are those two vaddrs? I think they manage it in the .system file by laying the two memory regions contiguous.

Oh, you must mean if there is only one client. That is fine since every example has its own ethernet_config.h file and net_mem_region_init_sys can be manually redefined.

Ivan-Velickovic commented 3 weeks ago

We should put the physical address initialisation inside net_mem_region_init_sys as well then.

Ivan-Velickovic commented 3 weeks ago

Hmm that probably won't work either.

Cheng-Li1 commented 3 weeks ago

Hmm that probably won't work either.

I am wondering if we leave init buffer_region_paddrs to function defined in ethernet_config.h, then we probably either define uintptr_t buffer_data_region_cli0_paddr and uintptr_t buffer_data_region_cli1_paddr in ethernet_config.h or calling a function in init along with those values?