In fore200e_change_qos, there is a race condition due to two consecutive
updates to the 'available_cell_rate' variable. If a read operation
occurs between these updates, an intermediate value might be read,
leading to potential bugs.
To fix this issue, 'available_cell_rate' should be adjusted in a single
operation, ensuring consistency and preventing any intermediate states
from being read.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Gui-Dong Han <[email protected]>
---
drivers/atm/fore200e.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/atm/fore200e.c b/drivers/atm/fore200e.c
index cb00f8244e41..d54e044d7542 100644
--- a/drivers/atm/fore200e.c
+++ b/drivers/atm/fore200e.c
@@ -1906,8 +1906,7 @@ fore200e_change_qos(struct atm_vcc* vcc,struct atm_qos* qos, int flags)
return -EAGAIN;
}
- fore200e->available_cell_rate += vcc->qos.txtp.max_pcr;
- fore200e->available_cell_rate -= qos->txtp.max_pcr;
+ fore200e->available_cell_rate += vcc->qos.txtp.max_pcr - qos->txtp.max_pcr;
mutex_unlock(&fore200e->rate_mtx);
--
2.34.1
> In fore200e_change_qos, there is a race condition due to two consecutive
> updates to the 'available_cell_rate' variable. If a read operation
> occurs between these updates, an intermediate value might be read,
> leading to potential bugs.
* Would you like to explain a bit more why you find the applied mutex
insufficient for data synchronisation aspects?
* Is any special analysis tool (like “BassCheck”) involved in such a contribution?
> To fix this issue, 'available_cell_rate' should be adjusted in a single
> operation, ensuring consistency and preventing any intermediate states
> from being read.
* Please improve the change description with an imperative wording.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc2#n94
* How do you think about to specify the name of the affected function
in the summary phrase?
* I would like to point out that similar source code adjustments can be achieved
also by the means of the semantic patch language (Coccinelle software).
Regards,
Markus
On Tue, Jun 11, 2024 at 11:54:10AM +0800, Gui-Dong Han wrote:
> In fore200e_change_qos, there is a race condition due to two consecutive
> updates to the 'available_cell_rate' variable. If a read operation
> occurs between these updates, an intermediate value might be read,
> leading to potential bugs.
>
> To fix this issue, 'available_cell_rate' should be adjusted in a single
> operation, ensuring consistency and preventing any intermediate states
> from being read.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Gui-Dong Han <[email protected]>
Hi Gui-Dong Han,
If there is a race involving writing and reading available_cell_rate,
then I believe there is still a race after your patch: if nothing protects
to protect available_cell_rate from being read while it is written then
that is true both before and after this patch.
Also, I would suggest that this is a very old and possibly unused driver.
If you wish to spend time on it I'd suggest that time go into
investigating if it is appropriate to remove the driver entirely.