If amdgpu_eeprom_read() returns a negative error code then the error handling checks:
if (res < buf_size) {
The problem is that "buf_size" is a u32 so negative values are type promoted to a high positive values and the condition is false. Fix this by changing the type of "buf_size" to int.
Fixes: 79beb6114014 ("drm/amdgpu: Optimize EEPROM RAS table I/O") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com --- It's hard for me to tell the exact upper bound that "buf_size" can be, but if it's over USHRT_MAX then we are well toasted.
drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c index fc70620369e4..f07a456506ef 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c @@ -978,9 +978,8 @@ const struct file_operations amdgpu_ras_debugfs_eeprom_table_ops = { static int __verify_ras_table_checksum(struct amdgpu_ras_eeprom_control *control) { struct amdgpu_device *adev = to_amdgpu_device(control); - int res; + int buf_size, res; u8 csum, *buf, *pp; - u32 buf_size;
buf_size = RAS_TABLE_HEADER_SIZE + control->ras_num_recs * RAS_TABLE_RECORD_SIZE;
The i2c_transfer() function returns negatives or else the number of messages transferred. This code does not work because ARRAY_SIZE() is type size_t and so that means negative values of "r" are type promoted to high positive values which are greater than the ARRAY_SIZE().
Fix this by changing the < to != which works regardless of type promotion.
Fixes: 6cda0af81a50 ("drm/amdgpu: Fixes to the AMDGPU EEPROM driver") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c index 4c3c65a5acae..4d9eb0137f8c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eeprom.c @@ -147,7 +147,7 @@ static int __amdgpu_eeprom_xfer(struct i2c_adapter *i2c_adap, u32 eeprom_addr, /* This constitutes a START-STOP transaction. */ r = i2c_transfer(i2c_adap, msgs, ARRAY_SIZE(msgs)); - if (r < ARRAY_SIZE(msgs)) + if (r != ARRAY_SIZE(msgs)) break;
if (!read) {
This error path needs to unlock before returning. While we're at it, the correct error code from copy_to_user() failure is -EFAULT, not -EINVAL.
Fixes: 9b790694a031 (""drm/amdgpu: RAS EEPROM table is now in debugfs) Signed-off-by: Dan Carpenter dan.carpenter@oracle.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c index f07a456506ef..3e33e85d8dbc 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c @@ -821,7 +821,7 @@ static ssize_t amdgpu_ras_debugfs_table_read(struct file *f, char __user *buf, struct amdgpu_ras *ras = amdgpu_ras_get_context(adev); struct amdgpu_ras_eeprom_control *control = &ras->eeprom_control; const size_t orig_size = size; - int res = -EINVAL; + int res = -EFAULT; size_t data_len;
mutex_lock(&control->ras_tbl_mutex); @@ -912,8 +912,10 @@ static ssize_t amdgpu_ras_debugfs_table_read(struct file *f, char __user *buf, record.retired_page);
data_len = min_t(size_t, rec_hdr_fmt_size - r, size); - if (copy_to_user(buf, &data[r], data_len)) - return -EINVAL; + if (copy_to_user(buf, &data[r], data_len)) { + res = -EFAULT; + goto Out; + } buf += data_len; size -= data_len; *pos += data_len;
If copy_to_user() fails then this should return -EFAULT instead of -EINVAL.
Fixes: 9b790694a031 ("drm/amdgpu: RAS EEPROM table is now in debugfs") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c index 3e33e85d8dbc..d2e5b2567bc1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c @@ -771,7 +771,7 @@ amdgpu_ras_debugfs_eeprom_size_read(struct file *f, char __user *buf, res = min_t(size_t, res, size);
if (copy_to_user(buf, &data[*pos], res)) - return -EINVAL; + return -EFAULT;
*pos += res;
@@ -950,7 +950,7 @@ amdgpu_ras_debugfs_eeprom_table_read(struct file *f, char __user *buf, res = min_t(size_t, res, size);
if (copy_to_user(buf, &data[*pos], res)) - return -EINVAL; + return -EFAULT;
*pos += res;
Series is, Reviewed-by: Luben Tuikov luben.tuikov@amd.com
Regards, Luben
On 2021-07-03 5:44 a.m., Dan Carpenter wrote:
If amdgpu_eeprom_read() returns a negative error code then the error handling checks:
if (res < buf_size) {
The problem is that "buf_size" is a u32 so negative values are type promoted to a high positive values and the condition is false. Fix this by changing the type of "buf_size" to int.
Fixes: 79beb6114014 ("drm/amdgpu: Optimize EEPROM RAS table I/O") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
It's hard for me to tell the exact upper bound that "buf_size" can be, but if it's over USHRT_MAX then we are well toasted.
drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c index fc70620369e4..f07a456506ef 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c @@ -978,9 +978,8 @@ const struct file_operations amdgpu_ras_debugfs_eeprom_table_ops = { static int __verify_ras_table_checksum(struct amdgpu_ras_eeprom_control *control) { struct amdgpu_device *adev = to_amdgpu_device(control);
- int res;
- int buf_size, res; u8 csum, *buf, *pp;
u32 buf_size;
buf_size = RAS_TABLE_HEADER_SIZE + control->ras_num_recs * RAS_TABLE_RECORD_SIZE;
Alex,
I think we should pull these through amd-staging-drm-next.
Regards, Luben
On 2021-07-04 11:18 a.m., Luben Tuikov wrote:
Series is, Reviewed-by: Luben Tuikov luben.tuikov@amd.com
Regards, Luben
On 2021-07-03 5:44 a.m., Dan Carpenter wrote:
If amdgpu_eeprom_read() returns a negative error code then the error handling checks:
if (res < buf_size) {
The problem is that "buf_size" is a u32 so negative values are type promoted to a high positive values and the condition is false. Fix this by changing the type of "buf_size" to int.
Fixes: 79beb6114014 ("drm/amdgpu: Optimize EEPROM RAS table I/O") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
It's hard for me to tell the exact upper bound that "buf_size" can be, but if it's over USHRT_MAX then we are well toasted.
drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c index fc70620369e4..f07a456506ef 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c @@ -978,9 +978,8 @@ const struct file_operations amdgpu_ras_debugfs_eeprom_table_ops = { static int __verify_ras_table_checksum(struct amdgpu_ras_eeprom_control *control) { struct amdgpu_device *adev = to_amdgpu_device(control);
- int res;
- int buf_size, res; u8 csum, *buf, *pp;
u32 buf_size;
buf_size = RAS_TABLE_HEADER_SIZE + control->ras_num_recs * RAS_TABLE_RECORD_SIZE;
dri-devel@lists.freedesktop.org