[PATCH v1 0/2] Remove BUG_ON() and fix -ve array indexing

[Rayagonda Kokatanur]

This patch series contains following changes,

1. Avoid use of BUG_ON to prevent kernel crash and return error instead.
2. Fix possible negative array indexing

Rayagonda Kokatanur (2):
async_tx: return error instead of BUG_ON
async_tx: fix possible negative array indexing

crypto/async_tx/async_raid6_recov.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)

[Rayagonda Kokatanur]

Return error upon failure instead of using BUG_ON().
BUG_ON() will crash the kernel.

Signed-off-by: Rayagonda Kokatanur <***@broadcom.com>
---
crypto/async_tx/async_raid6_recov.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/crypto/async_tx/async_raid6_recov.c b/crypto/async_tx/async_raid6_recov.c
index f249142ceac4..33f2a8f8c9f4 100644
--- a/crypto/async_tx/async_raid6_recov.c
+++ b/crypto/async_tx/async_raid6_recov.c
@@ -205,7 +205,9 @@ __2data_recov_5(int disks, size_t bytes, int faila, int failb,
good = i;
good_srcs++;
}
- BUG_ON(good_srcs > 1);
+
+ if (good_srcs > 1)
+ return NULL;

p = blocks[disks-2];
q = blocks[disks-1];
@@ -339,7 +341,9 @@ async_raid6_2data_recov(int disks, size_t bytes, int faila, int failb,
void *scribble = submit->scribble;
int non_zero_srcs, i;

- BUG_ON(faila == failb);
+ if (faila == failb)
+ return NULL;
+
if (failb < faila)
swap(faila, failb);

@@ -455,7 +459,8 @@ async_raid6_datap_recov(int disks, size_t bytes, int faila,
break;
}
}
- BUG_ON(good_srcs == 0);
+ if (good_srcs == 0)
+ return NULL;

p = blocks[disks-2];
q = blocks[disks-1];

[Dan Williams]

On Mon, Mar 16, 2020 at 11:16 PM Rayagonda Kokatanur

Post by Rayagonda Kokatanur
Return error upon failure instead of using BUG_ON().
BUG_ON() will crash the kernel.

I don't think this patch is worth it, it has 2 problems:

- These conversions are buggy. Upper layers assume that the calls will
fall back to synchronous operation internally if they return NULL.

- These assertions indicate a major programming error that should not
be silently ignored. They need to be WARN_ON_ONCE() at a minimum, but
only if the above issue is solved.

These assertions are validating the raid implementation in raid5.c
which has been correctly handling this path for several years. The
risk of the code reorganization to "fix" this is higher than the
benefit given zero reports of these actually triggering in production.

[Rayagonda Kokatanur]

Fix possible negative array index read in __2data_recov_5() function.

Signed-off-by: Rayagonda Kokatanur <***@broadcom.com>
---
crypto/async_tx/async_raid6_recov.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/async_tx/async_raid6_recov.c b/crypto/async_tx/async_raid6_recov.c
index 33f2a8f8c9f4..9cd016cb2d09 100644
--- a/crypto/async_tx/async_raid6_recov.c
+++ b/crypto/async_tx/async_raid6_recov.c
@@ -206,7 +206,7 @@ __2data_recov_5(int disks, size_t bytes, int faila, int failb,
good_srcs++;
}

- if (good_srcs > 1)
+ if ((good_srcs > 1) || (good < 0))
return NULL;

p = blocks[disks-2];

[Dan Williams]

On Mon, Mar 16, 2020 at 11:16 PM Rayagonda Kokatanur

Post by Rayagonda Kokatanur
Fix possible negative array index read in __2data_recov_5() function.
---
crypto/async_tx/async_raid6_recov.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/crypto/async_tx/async_raid6_recov.c b/crypto/async_tx/async_raid6_recov.c
index 33f2a8f8c9f4..9cd016cb2d09 100644
--- a/crypto/async_tx/async_raid6_recov.c
+++ b/crypto/async_tx/async_raid6_recov.c
@@ -206,7 +206,7 @@ __2data_recov_5(int disks, size_t bytes, int faila, int failb,
good_srcs++;
}
- if (good_srcs > 1)
+ if ((good_srcs > 1) || (good < 0))
return NULL;

Read the code again, I don't see how this can happen.