* [PATCH] cxgb4: fix undefined behavior in mem.c @ 2019-02-28 22:38 Shaobo He 2019-02-28 22:56 ` Bart Van Assche 2019-03-04 19:54 ` Jason Gunthorpe 0 siblings, 2 replies; 9+ messages in thread From: Shaobo He @ 2019-02-28 22:38 UTC (permalink / raw) To: linux-rdma; +Cc: shaobo, Steve Wise, Doug Ledford, Jason Gunthorpe, open list In function `c4iw_dealloc_mw`, variable mhp's value is printed after freed, which triggers undefined behavior according to this post: https://trust-in-soft.com/dangling-pointer-indeterminate/. This commit fixes it by swapping the order of `kfree` and `pr_debug`. Signed-off-by: Shaobo He <shaobo@cs.utah.edu> --- drivers/infiniband/hw/cxgb4/mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/infiniband/hw/cxgb4/mem.c b/drivers/infiniband/hw/cxgb4/mem.c index 7b76e6f..bb8e0bc 100644 --- a/drivers/infiniband/hw/cxgb4/mem.c +++ b/drivers/infiniband/hw/cxgb4/mem.c @@ -684,8 +684,8 @@ int c4iw_dealloc_mw(struct ib_mw *mw) mhp->wr_waitp); kfree_skb(mhp->dereg_skb); c4iw_put_wr_wait(mhp->wr_waitp); - kfree(mhp); pr_debug("ib_mw %p mmid 0x%x ptr %p\n", mw, mmid, mhp); + kfree(mhp); return 0; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] cxgb4: fix undefined behavior in mem.c 2019-02-28 22:38 [PATCH] cxgb4: fix undefined behavior in mem.c Shaobo He @ 2019-02-28 22:56 ` Bart Van Assche 2019-02-28 23:18 ` Shaobo He 2019-03-04 19:54 ` Jason Gunthorpe 1 sibling, 1 reply; 9+ messages in thread From: Bart Van Assche @ 2019-02-28 22:56 UTC (permalink / raw) To: Shaobo He, linux-rdma Cc: Steve Wise, Doug Ledford, Jason Gunthorpe, open list On Thu, 2019-02-28 at 15:38 -0700, Shaobo He wrote: > In function `c4iw_dealloc_mw`, variable mhp's value is printed after > freed, which triggers undefined behavior according to this post: > https://trust-in-soft.com/dangling-pointer-indeterminate/. > > This commit fixes it by swapping the order of `kfree` and `pr_debug`. > > Signed-off-by: Shaobo He <shaobo@cs.utah.edu> > --- > drivers/infiniband/hw/cxgb4/mem.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/hw/cxgb4/mem.c b/drivers/infiniband/hw/cxgb4/mem.c > index 7b76e6f..bb8e0bc 100644 > --- a/drivers/infiniband/hw/cxgb4/mem.c > +++ b/drivers/infiniband/hw/cxgb4/mem.c > @@ -684,8 +684,8 @@ int c4iw_dealloc_mw(struct ib_mw *mw) > mhp->wr_waitp); > kfree_skb(mhp->dereg_skb); > c4iw_put_wr_wait(mhp->wr_waitp); > - kfree(mhp); > pr_debug("ib_mw %p mmid 0x%x ptr %p\n", mw, mmid, mhp); > + kfree(mhp); > return 0; > } Please quote the relevant paragraphs from the C standard. All I have found about free() in ISO/IEC 9899:2017 is the following: Description The free function causes the space pointed to by ptr to be deallocated, that is, made available for further allocation. If ptr is a null pointer, no action occurs. Otherwise, if the argument does not match a pointer earlier returned by a memory management function, or if the space has been deallocated by a call to free or realloc, the behavior is undefined. That is not sufficient to claim that the above code triggers undefined behavior. Bart. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] cxgb4: fix undefined behavior in mem.c 2019-02-28 22:56 ` Bart Van Assche @ 2019-02-28 23:18 ` Shaobo He 2019-02-28 23:33 ` Bart Van Assche 2019-03-01 18:15 ` Christopher Lameter 0 siblings, 2 replies; 9+ messages in thread From: Shaobo He @ 2019-02-28 23:18 UTC (permalink / raw) To: Bart Van Assche, linux-rdma Cc: Steve Wise, Doug Ledford, Jason Gunthorpe, open list I can't afford a pdf version of the C standard. So I looked at the draft version used in the link I put in the commit message. It says (in 6.2.4:2), ``` The lifetime of an object is the portion of program execution during which storage is guaranteed to be reserved for it. An object exists, has a constant address, and retains its last-stored value throughout its lifetime. If an object is referred to outside of its lifetime, the behavior is undefined. The value of a pointer becomes indeterminate when the object it points to (or just past) reaches the end of its lifetime. ``` I couldn't find the definition of lifetime over a dynamically allocated object in the draft of C standard. I refer to this link (https://en.cppreference.com/w/c/language/lifetime) which suggests that the lifetime of an allocated object ends after the deallocation function is called upon it. I think maybe the more problematic issue is that the value of a freed pointer is intermediate. Shaobo On 2/28/19 3:56 PM, Bart Van Assche wrote: > On Thu, 2019-02-28 at 15:38 -0700, Shaobo He wrote: >> In function `c4iw_dealloc_mw`, variable mhp's value is printed after >> freed, which triggers undefined behavior according to this post: >> https://trust-in-soft.com/dangling-pointer-indeterminate/. >> >> This commit fixes it by swapping the order of `kfree` and `pr_debug`. >> >> Signed-off-by: Shaobo He <shaobo@cs.utah.edu> >> --- >> drivers/infiniband/hw/cxgb4/mem.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/infiniband/hw/cxgb4/mem.c b/drivers/infiniband/hw/cxgb4/mem.c >> index 7b76e6f..bb8e0bc 100644 >> --- a/drivers/infiniband/hw/cxgb4/mem.c >> +++ b/drivers/infiniband/hw/cxgb4/mem.c >> @@ -684,8 +684,8 @@ int c4iw_dealloc_mw(struct ib_mw *mw) >> mhp->wr_waitp); >> kfree_skb(mhp->dereg_skb); >> c4iw_put_wr_wait(mhp->wr_waitp); >> - kfree(mhp); >> pr_debug("ib_mw %p mmid 0x%x ptr %p\n", mw, mmid, mhp); >> + kfree(mhp); >> return 0; >> } > > Please quote the relevant paragraphs from the C standard. All I have found > about free() in ISO/IEC 9899:2017 is the following: > > Description > The free function causes the space pointed to by ptr to be deallocated, that > is, made available for further allocation. If ptr is a null pointer, no > action occurs. Otherwise, if the argument does not match a pointer earlier > returned by a memory management function, or if the space has been > deallocated by a call to free or realloc, the behavior is undefined. > > That is not sufficient to claim that the above code triggers undefined > behavior. > > Bart. > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] cxgb4: fix undefined behavior in mem.c 2019-02-28 23:18 ` Shaobo He @ 2019-02-28 23:33 ` Bart Van Assche 2019-02-28 23:57 ` Shaobo He 2019-03-01 18:15 ` Christopher Lameter 1 sibling, 1 reply; 9+ messages in thread From: Bart Van Assche @ 2019-02-28 23:33 UTC (permalink / raw) To: Shaobo He, linux-rdma Cc: Steve Wise, Doug Ledford, Jason Gunthorpe, open list On Thu, 2019-02-28 at 16:18 -0700, Shaobo He wrote: > I can't afford a pdf version of the C standard. So I looked at the draft version > used in the link I put in the commit message. It says (in 6.2.4:2), > > ``` > The lifetime of an object is the portion of program execution during which > storage is guaranteed to be reserved for it. An object exists, has a constant > address, and retains its last-stored value throughout its lifetime. If an object > is referred to outside of its lifetime, the behavior is undefined. The value of > a pointer becomes indeterminate when the object it points to (or just past) > reaches the end of its lifetime. > ``` > I couldn't find the definition of lifetime over a dynamically allocated object > in the draft of C standard. I refer to this link > (https://en.cppreference.com/w/c/language/lifetime) which suggests that the > lifetime of an allocated object ends after the deallocation function is called > upon it. > > I think maybe the more problematic issue is that the value of a freed pointer is > intermediate. In another section of the same draft I found the following: J.2 Undefined behavior [ ... ] The value of a pointer that refers to space deallocated by a call to the free or realloc function is used (7.22.3). Since the C standard explicitly refers to free() and realloc(), does that mean that that statement about undefined behavior does not apply to munmap() (for user space code) nor to kfree() (for kernel code)? Bart. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] cxgb4: fix undefined behavior in mem.c 2019-02-28 23:33 ` Bart Van Assche @ 2019-02-28 23:57 ` Shaobo He 2019-03-01 14:26 ` Doug Ledford 0 siblings, 1 reply; 9+ messages in thread From: Shaobo He @ 2019-02-28 23:57 UTC (permalink / raw) To: Bart Van Assche, linux-rdma Cc: Steve Wise, Doug Ledford, Jason Gunthorpe, open list Good catch. But if we agree on that memory management functions are those specified by the C standard, would it be OK to ignore so-called use after free or double free bugs for the kernel as C standard does not apply to kfree? On 2/28/19 4:33 PM, Bart Van Assche wrote: > On Thu, 2019-02-28 at 16:18 -0700, Shaobo He wrote: >> I can't afford a pdf version of the C standard. So I looked at the draft version >> used in the link I put in the commit message. It says (in 6.2.4:2), >> >> ``` >> The lifetime of an object is the portion of program execution during which >> storage is guaranteed to be reserved for it. An object exists, has a constant >> address, and retains its last-stored value throughout its lifetime. If an object >> is referred to outside of its lifetime, the behavior is undefined. The value of >> a pointer becomes indeterminate when the object it points to (or just past) >> reaches the end of its lifetime. >> ``` >> I couldn't find the definition of lifetime over a dynamically allocated object >> in the draft of C standard. I refer to this link >> (https://en.cppreference.com/w/c/language/lifetime) which suggests that the >> lifetime of an allocated object ends after the deallocation function is called >> upon it. >> >> I think maybe the more problematic issue is that the value of a freed pointer is >> intermediate. > > In another section of the same draft I found the following: > > J.2 Undefined behavior [ ... ] The value of a pointer that refers to space > deallocated by a call to the free or realloc function is used (7.22.3). > > Since the C standard explicitly refers to free() and realloc(), does that > mean that that statement about undefined behavior does not apply to munmap() > (for user space code) nor to kfree() (for kernel code)? > > Bart. > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] cxgb4: fix undefined behavior in mem.c 2019-02-28 23:57 ` Shaobo He @ 2019-03-01 14:26 ` Doug Ledford 2019-03-01 21:21 ` Shaobo He 0 siblings, 1 reply; 9+ messages in thread From: Doug Ledford @ 2019-03-01 14:26 UTC (permalink / raw) To: Shaobo He, Bart Van Assche, linux-rdma Cc: Steve Wise, Jason Gunthorpe, open list [-- Attachment #1: Type: text/plain, Size: 3158 bytes --] On Thu, 2019-02-28 at 16:57 -0700, Shaobo He wrote: > Good catch. But if we agree on that memory management functions are those > specified by the C standard, would it be OK to ignore so-called use after free > or double free bugs for the kernel as C standard does not apply to kfree? No, most kernel use-after-free bugs are real bugs. This one might be technically a bug by certain readings of the standard, but it's a non- issue. Real use-after-free bugs don't just look at the value of a local stack variable to get the memory's old address (which is what this does, and the same could be achieved and be totally in spec by doing this: old_ptr = mhp; kfree(mhp); pr_debug("%p\n", old_ptr);) Real use after free things would actually dereference the pointer to either read or write from the old memory region. That leads to data corruption or kernel data leaks. Plus, in this case, the purpose of printing the literal value of mhp is simply to provide a unique name for tracing purposes. Since kfree() doesn't alter the local stack variable, the name is still present in the local stack variable at the point you call pr_debug(). It could be fixed. It's not like this patch is wrong. But I wouldn't submit it this late in the -rc cycle, I'd just take it for next. > On 2/28/19 4:33 PM, Bart Van Assche wrote: > > On Thu, 2019-02-28 at 16:18 -0700, Shaobo He wrote: > > > I can't afford a pdf version of the C standard. So I looked at the draft version > > > used in the link I put in the commit message. It says (in 6.2.4:2), > > > > > > ``` > > > The lifetime of an object is the portion of program execution during which > > > storage is guaranteed to be reserved for it. An object exists, has a constant > > > address, and retains its last-stored value throughout its lifetime. If an object > > > is referred to outside of its lifetime, the behavior is undefined. The value of > > > a pointer becomes indeterminate when the object it points to (or just past) > > > reaches the end of its lifetime. > > > ``` > > > I couldn't find the definition of lifetime over a dynamically allocated object > > > in the draft of C standard. I refer to this link > > > (https://en.cppreference.com/w/c/language/lifetime) which suggests that the > > > lifetime of an allocated object ends after the deallocation function is called > > > upon it. > > > > > > I think maybe the more problematic issue is that the value of a freed pointer is > > > intermediate. > > > > In another section of the same draft I found the following: > > > > J.2 Undefined behavior [ ... ] The value of a pointer that refers to space > > deallocated by a call to the free or realloc function is used (7.22.3). > > > > Since the C standard explicitly refers to free() and realloc(), does that > > mean that that statement about undefined behavior does not apply to munmap() > > (for user space code) nor to kfree() (for kernel code)? > > > > Bart. > > -- Doug Ledford <dledford@redhat.com> GPG KeyID: B826A3330E572FDD Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] cxgb4: fix undefined behavior in mem.c 2019-03-01 14:26 ` Doug Ledford @ 2019-03-01 21:21 ` Shaobo He 0 siblings, 0 replies; 9+ messages in thread From: Shaobo He @ 2019-03-01 21:21 UTC (permalink / raw) To: Doug Ledford, Bart Van Assche, linux-rdma Cc: Steve Wise, Jason Gunthorpe, open list, cl Yes, why wouldn't they be real bugs? I was simply pointing out the irrational conclusion if the C standard is strictly applied to kernel code. I think the spirit of the C standard is that one shouldn't rely on the assumption that the value of a freed pointer does not change, even though in practice any compiler developers probably won't bother to implement the logic to change the pointer value or optimizations leveraging it even they are allowed to. In other words, the original code may be a little bit problematic in the spirit of the C standard whereas the patch simply makes it totally valid. If it can be finally submitted, that would be great. If not, I'm totally fine. Shaobo On 2019/3/1 7:26, Doug Ledford wrote: > On Thu, 2019-02-28 at 16:57 -0700, Shaobo He wrote: >> Good catch. But if we agree on that memory management functions are those >> specified by the C standard, would it be OK to ignore so-called use after free >> or double free bugs for the kernel as C standard does not apply to kfree? > > No, most kernel use-after-free bugs are real bugs. This one might be > technically a bug by certain readings of the standard, but it's a non- > issue. Real use-after-free bugs don't just look at the value of a local > stack variable to get the memory's old address (which is what this does, > and the same could be achieved and be totally in spec by doing this: > > old_ptr = mhp; > kfree(mhp); > pr_debug("%p\n", old_ptr);) > > Real use after free things would actually dereference the pointer to > either read or write from the old memory region. That leads to data > corruption or kernel data leaks. Plus, in this case, the purpose of > printing the literal value of mhp is simply to provide a unique name for > tracing purposes. Since kfree() doesn't alter the local stack variable, > the name is still present in the local stack variable at the point you > call pr_debug(). > > It could be fixed. It's not like this patch is wrong. But I wouldn't > submit it this late in the -rc cycle, I'd just take it for next. > >> On 2/28/19 4:33 PM, Bart Van Assche wrote: >>> On Thu, 2019-02-28 at 16:18 -0700, Shaobo He wrote: >>>> I can't afford a pdf version of the C standard. So I looked at the draft version >>>> used in the link I put in the commit message. It says (in 6.2.4:2), >>>> >>>> ``` >>>> The lifetime of an object is the portion of program execution during which >>>> storage is guaranteed to be reserved for it. An object exists, has a constant >>>> address, and retains its last-stored value throughout its lifetime. If an object >>>> is referred to outside of its lifetime, the behavior is undefined. The value of >>>> a pointer becomes indeterminate when the object it points to (or just past) >>>> reaches the end of its lifetime. >>>> ``` >>>> I couldn't find the definition of lifetime over a dynamically allocated object >>>> in the draft of C standard. I refer to this link >>>> (https://en.cppreference.com/w/c/language/lifetime) which suggests that the >>>> lifetime of an allocated object ends after the deallocation function is called >>>> upon it. >>>> >>>> I think maybe the more problematic issue is that the value of a freed pointer is >>>> intermediate. >>> >>> In another section of the same draft I found the following: >>> >>> J.2 Undefined behavior [ ... ] The value of a pointer that refers to space >>> deallocated by a call to the free or realloc function is used (7.22.3). >>> >>> Since the C standard explicitly refers to free() and realloc(), does that >>> mean that that statement about undefined behavior does not apply to munmap() >>> (for user space code) nor to kfree() (for kernel code)? >>> >>> Bart. >>> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] cxgb4: fix undefined behavior in mem.c 2019-02-28 23:18 ` Shaobo He 2019-02-28 23:33 ` Bart Van Assche @ 2019-03-01 18:15 ` Christopher Lameter 1 sibling, 0 replies; 9+ messages in thread From: Christopher Lameter @ 2019-03-01 18:15 UTC (permalink / raw) To: Shaobo He Cc: Bart Van Assche, linux-rdma, Steve Wise, Doug Ledford, Jason Gunthorpe, open list On Thu, 28 Feb 2019, Shaobo He wrote: > I think maybe the more problematic issue is that the value of a freed pointer > is intermediate. The pointer is not affected by freeing the data it points to. Thus it definitely has the same value as before and is not indeterminate. The pointer points now to an area of memory that could now be in use for different purposes so maybe it could be taken as a dangerous situation. But situations like that are common in code. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] cxgb4: fix undefined behavior in mem.c 2019-02-28 22:38 [PATCH] cxgb4: fix undefined behavior in mem.c Shaobo He 2019-02-28 22:56 ` Bart Van Assche @ 2019-03-04 19:54 ` Jason Gunthorpe 1 sibling, 0 replies; 9+ messages in thread From: Jason Gunthorpe @ 2019-03-04 19:54 UTC (permalink / raw) To: Shaobo He; +Cc: linux-rdma, Steve Wise, Doug Ledford, open list On Thu, Feb 28, 2019 at 03:38:38PM -0700, Shaobo He wrote: > In function `c4iw_dealloc_mw`, variable mhp's value is printed after > freed, which triggers undefined behavior according to this post: > https://trust-in-soft.com/dangling-pointer-indeterminate/. > > This commit fixes it by swapping the order of `kfree` and `pr_debug`. > > Signed-off-by: Shaobo He <shaobo@cs.utah.edu> > --- > drivers/infiniband/hw/cxgb4/mem.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Discussion aside, this is a worthwile fix. I rewrote the commit message to avoid referencing 'undefined behavior' though, this is just a straight up bug in the logging. Another thread could get the same pointer value for the mhp before the print creating a confusing log. cxgb4: kfree mhp after the debug print In function `c4iw_dealloc_mw`, variable mhp's value is printed after freed, it is clearer to have the print before the kfree. Otherwise racing threads could allocate another mhp with the same pointer value and create confusing tracing. Jason ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-03-04 19:54 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-28 22:38 [PATCH] cxgb4: fix undefined behavior in mem.c Shaobo He 2019-02-28 22:56 ` Bart Van Assche 2019-02-28 23:18 ` Shaobo He 2019-02-28 23:33 ` Bart Van Assche 2019-02-28 23:57 ` Shaobo He 2019-03-01 14:26 ` Doug Ledford 2019-03-01 21:21 ` Shaobo He 2019-03-01 18:15 ` Christopher Lameter 2019-03-04 19:54 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).