| Message ID | 1524045904-7005-1-git-send-email-amit.pundir@linaro.org |
|---|---|
| Headers | show |
| Series | Few NFC fixes from android-4.14 tree | expand |
On Wed, 2018-04-18 at 15:35 +0530, Amit Pundir wrote: > if (skb->data[transaction->aid_len + 2] != > - NFC_EVT_TRANSACTION_PARAMS_TAG) > + NFC_EVT_TRANSACTION_PARAMS_TAG || > + skb->len < transaction->aid_len + transaction- > >params_len + 4) { > + devm_kfree(dev, transaction); Oh, no. This is not memory leak per se, this is bad choice of devm_ API where it should use plain kmalloc() / kfree(). > return -EPROTO; > + } -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy
On Fri, Apr 20, 2018 at 03:39:46PM +0300, Andy Shevchenko wrote: > On Wed, 2018-04-18 at 15:35 +0530, Amit Pundir wrote: > > > if (skb->data[transaction->aid_len + 2] != > > - NFC_EVT_TRANSACTION_PARAMS_TAG) > > + NFC_EVT_TRANSACTION_PARAMS_TAG || > > + skb->len < transaction->aid_len + transaction- > > >params_len + 4) { > > > + devm_kfree(dev, transaction); > > Oh, no. > > This is not memory leak per se, this is bad choice of devm_ API where it > should use plain kmalloc() / kfree(). Also, there is no check to see if the allocation worked at all. Mark --
On 20 April 2018 at 18:09, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, 2018-04-18 at 15:35 +0530, Amit Pundir wrote: > >> if (skb->data[transaction->aid_len + 2] != >> - NFC_EVT_TRANSACTION_PARAMS_TAG) >> + NFC_EVT_TRANSACTION_PARAMS_TAG || >> + skb->len < transaction->aid_len + transaction- >> >params_len + 4) { > >> + devm_kfree(dev, transaction); > > Oh, no. > > This is not memory leak per se, this is bad choice of devm_ API where it > should use plain kmalloc() / kfree(). > Hi, If I switch to kmalloc()/kfree() with allocation and may be pre-usage checks along the way up to nfc_genl_se_transaction() would that suffice? I believe, I still be needing the additional aid_len and params_len checks regardless, right? Regards, Amit Pundir >> return -EPROTO; >> + } > > -- > Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Intel Finland Oy
On 20 April 2018 at 22:15, Mark Greer <mgreer@animalcreek.com> wrote: > On Fri, Apr 20, 2018 at 03:39:46PM +0300, Andy Shevchenko wrote: >> On Wed, 2018-04-18 at 15:35 +0530, Amit Pundir wrote: >> >> > if (skb->data[transaction->aid_len + 2] != >> > - NFC_EVT_TRANSACTION_PARAMS_TAG) >> > + NFC_EVT_TRANSACTION_PARAMS_TAG || >> > + skb->len < transaction->aid_len + transaction- >> > >params_len + 4) { >> >> > + devm_kfree(dev, transaction); >> >> Oh, no. >> >> This is not memory leak per se, this is bad choice of devm_ API where it >> should use plain kmalloc() / kfree(). > > Also, there is no check to see if the allocation worked at all. Ack. I'll add that in v2. Thanks. Regards, Amit Pundir > > Mark > --
Hi, Resending few NFC fixes I picked up from android-4.14 tree[1] for review and comments. They seem reasonable upstream candidates. My last attempt was not timed properly and it got lost between Christmas-New Year break and then Meltdown-Spectre happened. Also like to point out that I have not feature tested these patches at all. Only made small cosmetic changes to the original patches (removed Android-only tag and internal bug ID) and build tested for arm/arm64 defconfigs, before posting them here for review. Really appreciate any comments or feedback on how to take it forward. Regards, Amit Pundir [1] https://android.googlesource.com/kernel/common/+log/android-4.14 Suren Baghdasaryan (4): NFC: st21nfca: Fix out of bounds kernel access when handling ATR_REQ NFC: st21nfca: Fix memory OOB and leak issues in connectivity events handler NFC: Fix possible memory corruption when handling SHDLC I-Frame commands NFC: fdp: Fix possible buffer overflow in WCS4000 NFC driver drivers/nfc/fdp/i2c.c | 10 ++++++++++ drivers/nfc/st21nfca/dep.c | 3 ++- drivers/nfc/st21nfca/se.c | 18 ++++++++++++++---- net/nfc/hci/core.c | 10 ++++++++++ 4 files changed, 36 insertions(+), 5 deletions(-) -- 2.7.4