Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Out-of-bounds read in VBIOS parsing due to missing bounds checks on `data_offset` against `bios_size`. - [High] Memory and resource leak of `struct amdgpu_i2c_chan` objects and I2C adapters during driver teardown. - [High] Unchecked VBIOS size subtraction causes unsigned underflow and unconstrained loops in other ATOM parser functions. -- commit 07534aa5f05a4ba05838a80933016849ebced1e1 Author: Candice Li <[email protected]> drm/amdgpu: Bound GPIO I2C table entry count from VBIOS This commit caps the parsed GPIO I2C table entry count to AMDGPU_MAX_I2C_BUS to prevent array overruns and unbounded iteration on corrupt VBIOS metadata. [ Upstream commit a99cd231cd924b7160fecb9fb3a94b801522323b ] > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c > index 763f2b8dcf13a..b8f7e3a18d324 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c [ ... ] > @@ -127,8 +141,7 @@ void amdgpu_atombios_i2c_init(struct amdgpu_device *adev) > if (amdgpu_atom_parse_data_header(ctx, index, &size, NULL, NULL, > &data_offset)) { > i2c_info = (struct _ATOM_GPIO_I2C_INFO *)(ctx->bios + > data_offset); [Severity: High] This isn't a bug introduced by this patch, but does this code potentially dereference an out-of-bounds pointer if the parsed data_offset exceeds the size of the VBIOS allocation? If the VBIOS data is malformed and data_offset is larger than the actual allocation, ctx->bios + data_offset could result in an out-of-bounds read when i2c_info is dereferenced. The unvalidated data_offset originates from: drivers/gpu/drm/amd/amdgpu/atom.c:amdgpu_atom_parse_data_header() { ... int offset = index * 2 + 4; int idx = CU16(ctx->data_table + offset); ... *data_start = idx; return true; } > - num_indices = (size - sizeof(ATOM_COMMON_TABLE_HEADER)) / > - sizeof(ATOM_GPIO_I2C_ASSIGMENT); > + num_indices = amdgpu_atombios_gpio_i2c_num_entries(size); [ ... ] I also noticed a pre-existing resource management issue during driver teardown: drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c:amdgpu_i2c_create() { ... i2c = kzalloc(sizeof(struct amdgpu_i2c_chan), GFP_KERNEL); if (i2c == NULL) return NULL; ... ret = i2c_bit_add_bus(&i2c->adapter); ... } drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c:amdgpu_i2c_fini() { int i; for (i = 0; i < AMDGPU_MAX_I2C_BUS; i++) if (adev->i2c_bus[i]) adev->i2c_bus[i] = NULL; } [Severity: High] This is a pre-existing issue, but does this code leak the struct amdgpu_i2c_chan object and the bit-banging I2C adapter registration? When amdgpu_i2c_fini() cleans up the I2C buses, it sets adev->i2c_bus[i] to NULL but does not appear to call kfree() on the struct amdgpu_i2c_chan, nor does it call i2c_del_adapter() for the adapters registered with i2c_bit_add_bus(), which aren't managed by devres. Could this leave dangling I2C adapters and permanently leak the kzalloc'd memory? Another pre-existing issue in the same file involves a similar underflow: drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:amdgpu_atombios_lookup_gpio() { ... if (amdgpu_atom_parse_data_header(ctx, index, &size, NULL, NULL, &data_offset)) { gpio_info = (struct _ATOM_GPIO_PIN_LUT *)(ctx->bios + data_offset); num_indices = (size - sizeof(ATOM_COMMON_TABLE_HEADER)) / sizeof(ATOM_GPIO_PIN_ASSIGNMENT); ... } [Severity: High] This isn't a bug introduced by this patch, but does this code overflow num_indices if size is less than 4 bytes? Since size is a u16 and sizeof(ATOM_COMMON_TABLE_HEADER) is 4, a size smaller than 4 will cause the subtraction to underflow in an unsigned context. Could this result in a massive num_indices value, causing the subsequent loop to read far beyond the bounds of the VBIOS allocation? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
