| Message ID | 1453830820-27104-1-git-send-email-xiyou.wangcong@gmail.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Cong, On Wed, Jan 27, 2016 at 4:53 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: A commit message would be nice. A brief rundown of how this is called from userspace would be nice (I'm talking a single sentence here, e.g. "this is allocated when submitting a nfc packet") and what issue __GFP_NOWARN is fixing. (I'm guessing log spam due to allocation failures.) > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Cc: Lauro Ramos Venancio <lauro.venancio@openbossa.org> > Cc: Aloisio Almeida Jr <aloisio.almeida@openbossa.org> > Cc: Samuel Ortiz <sameo@linux.intel.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > net/nfc/llcp_commands.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/nfc/llcp_commands.c b/net/nfc/llcp_commands.c > index 3621a90..5d94055 100644 > --- a/net/nfc/llcp_commands.c > +++ b/net/nfc/llcp_commands.c > @@ -729,7 +729,7 @@ int nfc_llcp_send_ui_frame(struct nfc_llcp_sock *sock, u8 ssap, u8 dsap, > if (local == NULL) > return -ENODEV; > > - msg_data = kzalloc(len, GFP_KERNEL); > + msg_data = kzalloc(len, GFP_USER | __GFP_NOWARN); > if (msg_data == NULL) > return -ENOMEM; Thanks,
On Tue, Jan 26, 2016 at 2:55 PM, Julian Calaby <julian.calaby@gmail.com> wrote: > Hi Cong, > > On Wed, Jan 27, 2016 at 4:53 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > > A commit message would be nice. A brief rundown of how this is called > from userspace would be nice (I'm talking a single sentence here, e.g. > "this is allocated when submitting a nfc packet") and what issue > __GFP_NOWARN is fixing. (I'm guessing log spam due to allocation > failures.) > I thought it is obvious. ;) Keep in mind that $subject is one part of commit message too, so there is a commit message although very short. I will add it.
Hi Cong, On Wed, Jan 27, 2016 at 10:12 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Tue, Jan 26, 2016 at 2:55 PM, Julian Calaby <julian.calaby@gmail.com> wrote: >> Hi Cong, >> >> On Wed, Jan 27, 2016 at 4:53 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> >> A commit message would be nice. A brief rundown of how this is called >> from userspace would be nice (I'm talking a single sentence here, e.g. >> "this is allocated when submitting a nfc packet") and what issue >> __GFP_NOWARN is fixing. (I'm guessing log spam due to allocation >> failures.) >> > > I thought it is obvious. ;) Keep in mind that $subject is one part of commit > message too, so there is a commit message although very short. > > I will add it. I know almost nothing about how the NFC subsystem works, and this looks like a potential security issue, so more information is better IMHO. Thanks,
On Tue, 2016-01-26 at 15:12 -0800, Cong Wang wrote: > On Tue, Jan 26, 2016 at 2:55 PM, Julian Calaby <julian.calaby@gmail.com> wrote: > > Hi Cong, > > > > On Wed, Jan 27, 2016 at 4:53 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > A commit message would be nice. A brief rundown of how this is called > > from userspace would be nice (I'm talking a single sentence here, e.g. > > "this is allocated when submitting a nfc packet") and what issue > > __GFP_NOWARN is fixing. (I'm guessing log spam due to allocation > > failures.) > > > > I thought it is obvious. ;) Keep in mind that $subject is one part of commit > message too, so there is a commit message although very short. > > I will add it. BTW, kzalloc() is useless here, since it is followed by if (memcpy_from_msg(msg_data, msg, len)) { Also, this file seems to have two spots with the same problem, in nfc_llcp_send_ui_frame() & nfc_llcp_send_i_frame()
On Tue, Jan 26, 2016 at 6:23 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Tue, 2016-01-26 at 15:12 -0800, Cong Wang wrote: >> On Tue, Jan 26, 2016 at 2:55 PM, Julian Calaby <julian.calaby@gmail.com> wrote: >> > Hi Cong, >> > >> > On Wed, Jan 27, 2016 at 4:53 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> > >> > A commit message would be nice. A brief rundown of how this is called >> > from userspace would be nice (I'm talking a single sentence here, e.g. >> > "this is allocated when submitting a nfc packet") and what issue >> > __GFP_NOWARN is fixing. (I'm guessing log spam due to allocation >> > failures.) >> > >> >> I thought it is obvious. ;) Keep in mind that $subject is one part of commit >> message too, so there is a commit message although very short. >> >> I will add it. > > > BTW, kzalloc() is useless here, since it is followed by > > if (memcpy_from_msg(msg_data, msg, len)) { Hmm? I think nfc_llcp_send_ui_frame() needs to do some fragmention with this temporary memory, or you are saying msg_iter has some API available to seek the pointer? Even if so, it doesn't look like suitable for -stable. > > Also, this file seems to have two spots with the same problem, > in nfc_llcp_send_ui_frame() & nfc_llcp_send_i_frame() > Ah, yes, I missed nfc_llcp_send_i_frame().
On Wed, 2016-01-27 at 11:50 -0800, Cong Wang wrote: > Hmm? I think nfc_llcp_send_ui_frame() needs to do some fragmention > with this temporary memory, or you are saying msg_iter has some > API available to seek the pointer? Even if so, it doesn't look like > suitable for -stable. > memcpy_from_msg(msg_data, msg, len) will overwrite the msg_data with len bytes, or return an error. So prior msg_data content does not matter. kzalloc() before a memset() or memcpy() sounds defensive programming, kmalloc() is a bit faster.
On Wed, Jan 27, 2016 at 1:05 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Wed, 2016-01-27 at 11:50 -0800, Cong Wang wrote: > >> Hmm? I think nfc_llcp_send_ui_frame() needs to do some fragmention >> with this temporary memory, or you are saying msg_iter has some >> API available to seek the pointer? Even if so, it doesn't look like >> suitable for -stable. >> > > memcpy_from_msg(msg_data, msg, len) will overwrite the msg_data with len > bytes, or return an error. > > So prior msg_data content does not matter. > > kzalloc() before a memset() or memcpy() sounds defensive programming, > kmalloc() is a bit faster. Oh, you mean s/kzalloc/kmalloc/, I thought you mean s/kzalloc//. ;)
diff --git a/net/nfc/llcp_commands.c b/net/nfc/llcp_commands.c index 3621a90..5d94055 100644 --- a/net/nfc/llcp_commands.c +++ b/net/nfc/llcp_commands.c @@ -729,7 +729,7 @@ int nfc_llcp_send_ui_frame(struct nfc_llcp_sock *sock, u8 ssap, u8 dsap, if (local == NULL) return -ENODEV; - msg_data = kzalloc(len, GFP_KERNEL); + msg_data = kzalloc(len, GFP_USER | __GFP_NOWARN); if (msg_data == NULL) return -ENOMEM;
Reported-by: Dmitry Vyukov <dvyukov@google.com> Cc: Lauro Ramos Venancio <lauro.venancio@openbossa.org> Cc: Aloisio Almeida Jr <aloisio.almeida@openbossa.org> Cc: Samuel Ortiz <sameo@linux.intel.com> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/nfc/llcp_commands.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)