| Message ID | Y/+hVSSFgeV+yPhY@ubun2204.myguest.virtualbox.org (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [RESEND] scsi: libfc: Use refcount_* APIs for reference count management | expand |
On Thu, 2023-03-02 at 00:32 +0530, Deepak R Varma wrote: > The atomic_t API based object reference counter management is prone > to counter value overflows, object use-after-free issues and to > return puzzling values. The improved refcount_t APIs are designed to > address these known issues with atomic_t reference counter > management. This white paper [1] has detailed reasons for moving from > atomic_t to refcount_t APIs. Hence replace the atomic_* based > implementation by its refcount_* based equivalent. > The issue is identified using atomic_as_refcounter.cocci Coccinelle > semantic patch script. > > [1] https://arxiv.org/pdf/1710.06175.pdf Citing long whitepapers in support of a patch isn't helpful to time pressed reviewers, particularly when it's evident you didn't understand the paper you cite. The argument in the paper for replacing atomics with refcounts can be summarized as: if a user can cause a counter overflow in an atomic_t simply by performing some action from userspace then that represents a source of potential overflow attacks on the kernel which should be mitigated by replacing the atomic_t in question with a refcount_t which is overflow resistant. What's missing from the quoted changelog is a justification of how a user could cause an overflow in the ex_refcnt atomic_t. James
On Wed, Mar 01, 2023 at 02:28:49PM -0500, James Bottomley wrote: > On Thu, 2023-03-02 at 00:32 +0530, Deepak R Varma wrote: > > The atomic_t API based object reference counter management is prone > > to counter value overflows, object use-after-free issues and to > > return puzzling values. The improved refcount_t APIs are designed to > > address these known issues with atomic_t reference counter > > management. This white paper [1] has detailed reasons for moving from > > atomic_t to refcount_t APIs. Hence replace the atomic_* based > > implementation by its refcount_* based equivalent. > > The issue is identified using atomic_as_refcounter.cocci Coccinelle > > semantic patch script. > > > > [1] https://arxiv.org/pdf/1710.06175.pdf > > Citing long whitepapers in support of a patch isn't helpful to time > pressed reviewers, particularly when it's evident you didn't understand > the paper you cite. The argument in the paper for replacing atomics > with refcounts can be summarized as: if a user can cause a counter > overflow in an atomic_t simply by performing some action from userspace > then that represents a source of potential overflow attacks on the > kernel which should be mitigated by replacing the atomic_t in question > with a refcount_t which is overflow resistant. > > What's missing from the quoted changelog is a justification of how a > user could cause an overflow in the ex_refcnt atomic_t. Thank you very much James for the review comments. I truly appreciate your time and guidance. I will study your feedback and send in a revision with necessary update to patch log. Regards, ./drv > > James >
diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c index 1d91c457527f..1c49fddb65e3 100644 --- a/drivers/scsi/libfc/fc_exch.c +++ b/drivers/scsi/libfc/fc_exch.c @@ -246,7 +246,7 @@ static const char *fc_exch_rctl_name(unsigned int op) */ static inline void fc_exch_hold(struct fc_exch *ep) { - atomic_inc(&ep->ex_refcnt); + refcount_inc(&ep->ex_refcnt); } /** @@ -312,7 +312,7 @@ static void fc_exch_release(struct fc_exch *ep) { struct fc_exch_mgr *mp; - if (atomic_dec_and_test(&ep->ex_refcnt)) { + if (refcount_dec_and_test(&ep->ex_refcnt)) { mp = ep->em; if (ep->destructor) ep->destructor(&ep->seq, ep->arg); @@ -329,7 +329,7 @@ static inline void fc_exch_timer_cancel(struct fc_exch *ep) { if (cancel_delayed_work(&ep->timeout_work)) { FC_EXCH_DBG(ep, "Exchange timer canceled\n"); - atomic_dec(&ep->ex_refcnt); /* drop hold for timer */ + refcount_dec(&ep->ex_refcnt); /* drop hold for timer */ } } @@ -1897,7 +1897,7 @@ static void fc_exch_reset(struct fc_exch *ep) ep->state |= FC_EX_RST_CLEANUP; fc_exch_timer_cancel(ep); if (ep->esb_stat & ESB_ST_REC_QUAL) - atomic_dec(&ep->ex_refcnt); /* drop hold for rec_qual */ + refcount_dec(&ep->ex_refcnt); /* drop hold for rec_qual */ ep->esb_stat &= ~ESB_ST_REC_QUAL; sp = &ep->seq; rc = fc_exch_done_locked(ep); @@ -2332,7 +2332,7 @@ static void fc_exch_els_rrq(struct fc_frame *fp) */ if (ep->esb_stat & ESB_ST_REC_QUAL) { ep->esb_stat &= ~ESB_ST_REC_QUAL; - atomic_dec(&ep->ex_refcnt); /* drop hold for rec qual */ + refcount_dec(&ep->ex_refcnt); /* drop hold for rec qual */ } if (ep->esb_stat & ESB_ST_COMPLETE) fc_exch_timer_cancel(ep); diff --git a/include/scsi/libfc.h b/include/scsi/libfc.h index 6e29e1719db1..ce65149b300c 100644 --- a/include/scsi/libfc.h +++ b/include/scsi/libfc.h @@ -432,7 +432,7 @@ struct fc_seq { */ struct fc_exch { spinlock_t ex_lock; - atomic_t ex_refcnt; + refcount_t ex_refcnt; enum fc_class class; struct fc_exch_mgr *em; struct fc_exch_pool *pool;
The atomic_t API based object reference counter management is prone to counter value overflows, object use-after-free issues and to return puzzling values. The improved refcount_t APIs are designed to address these known issues with atomic_t reference counter management. This white paper [1] has detailed reasons for moving from atomic_t to refcount_t APIs. Hence replace the atomic_* based implementation by its refcount_* based equivalent. The issue is identified using atomic_as_refcounter.cocci Coccinelle semantic patch script. [1] https://arxiv.org/pdf/1710.06175.pdf Signed-off-by: Deepak R Varma <drv@mailo.com> --- Note: The proposal is compile tested only. Resending the patch for review and feedback. drivers/scsi/libfc/fc_exch.c | 10 +++++----- include/scsi/libfc.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-)